-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ES|QL] Substitute date_trunc with round_to when the pre-calculated rounding points are available #128639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ES|QL] Substitute date_trunc with round_to when the pre-calculated rounding points are available #128639
Conversation
|
Hi @fang-xing-esql, I've created a changelog YAML for you. |
…r multiple quantities for month/quarter/year
…r multiple quantities for month/quarter/year
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
To call it out - if your data is clean - which most log indices are - then you'll see the ~12% performance improvement from this if you are CPU bound. There's a few follow ups coming:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me but probably wants someone with more QL background too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general it looks good, I think the surrogate expression approach can be used for this kind of replacement.
I have objections regarding code abstractions and readability. The code in Bucket and DateTrunc is almost identical with minor exception and can be refactored and simplified.
Also, the SurrogateExpression that we use today is specific to the LogicalPlanOptimizer and is used only there. I would create a separate interface only for things applicable on data nodes, even more so since this new rule will use SearchStats (which is a data node thing).
...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateTrunc.java
Outdated
Show resolved
Hide resolved
| var min = searchStats.min(fieldName); | ||
| var max = searchStats.max(fieldName); | ||
| // If min/max is available create rounding with them | ||
| if (min instanceof Long minValue && max instanceof Long maxValue && interval().foldable()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
min and max here can be null? Can they be something else other than Long? I am assuming yes, and if so why here it only matters if it's long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, min and max can be null if we don't find statistics of the fields from Lucene. And in this PR, they can only be Long, because:
SearchStatsonly consolidatesminandmaxfordatefields, and they areLongin Lucene.- We only do the substitution for
datefields inDateTruncandBuckethere.
...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateTrunc.java
Outdated
Show resolved
Hide resolved
...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateTrunc.java
Outdated
Show resolved
Hide resolved
...sql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java
Outdated
Show resolved
Hide resolved
.../xpack/esql/optimizer/rules/logical/local/SubstituteSurrogateExpressionsWithSearchStats.java
Outdated
Show resolved
Hide resolved
...gin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/Bucket.java
Outdated
Show resolved
Hide resolved
...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateTrunc.java
Outdated
Show resolved
Hide resolved
...gin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/Bucket.java
Outdated
Show resolved
Hide resolved
...gin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/Bucket.java
Show resolved
Hide resolved
|
|
||
| private final ZoneId zoneId; | ||
|
|
||
| protected BinaryDateTimeFunction(Source source, Expression argument, Expression timestamp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean up, this class is not used.
| public class DateDiff extends EsqlScalarFunction { | ||
| public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(Expression.class, "DateDiff", DateDiff::new); | ||
|
|
||
| public static final ZoneId UTC = ZoneId.of("Z"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExpressionBuilder, Add and Sub all refer to DateUtils.UTC, which is the same as ZoneId.of("Z"), refactor this to point to DateUtils.UTC as well. This seems to be the only place that directly references ZoneId.of("Z").
| private LogicalPlan plan(String query, Analyzer analyzer) { | ||
| var analyzed = analyzer.analyze(parser.createStatement(query, EsqlTestUtils.TEST_CFG)); | ||
| // System.out.println(analyzed); | ||
| var optimized = logicalOptimizer.optimize(analyzed); | ||
| // System.out.println(optimized); | ||
| return optimized; | ||
| return logicalOptimizer.optimize(analyzed); | ||
| } | ||
|
|
||
| private LogicalPlan plan(String query) { | ||
| protected LogicalPlan plan(String query) { | ||
| return plan(query, analyzer); | ||
| } | ||
|
|
||
| private LogicalPlan localPlan(LogicalPlan plan, SearchStats searchStats) { | ||
| protected LogicalPlan localPlan(LogicalPlan plan, SearchStats searchStats) { | ||
| var localContext = new LocalLogicalOptimizerContext(EsqlTestUtils.TEST_CFG, FoldContext.small(), searchStats); | ||
| // System.out.println(plan); | ||
| var localPlan = new LocalLogicalPlanOptimizer(localContext).localOptimize(plan); | ||
| // System.out.println(localPlan); | ||
| return localPlan; | ||
| return new LocalLogicalPlanOptimizer(localContext).localOptimize(plan); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small refactors.
Thanks so much for reviewing and for the suggestions on the code refactor @astefan! All of the comments have been addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Is there any existent IT test (csv-spec or yaml) that covers queries that are impacted by this change?
I mean, it feels right to also see this thing in a more real test (like something closer to what users will use and see and feel). If there is no such IT test, please add one which makes sense and which actually uses this thing this PR adds.
|
For checking that things are actually rewritten in a test, you can do a new test case in Once we push this stuff to the search index we can use the infrastructure in |
But that's a followup. |
We have quite a few existing |
For me this kind of thing is mostly to make sure that we're really turning on the optimization when we expect to. It's not about today so much - it's about a year from now when you go and make another change and it turns off the optimization unexpectedly. |
It's good that we have them. I'm ok with the PR as is now, no need for another IT test. Let's merge it 👍 |
This is tracked in #131341
|
This is a subtask of the filter-by-filter aggregation in
ES|QL. And the following changes are included in this PR.SearchStats, according to the statistics retrieved from Lucene forDateFieldTypeonly, addedSearchContextStatsTestsand testedmin/max.min/maxare available inSearchStats, provide them toDateTrunc.createRounding, callprepare(min, max), instead ofprepareForUnknown().fixedRoundingPoints(), substitutedate_truncandbucketwithround_tofunction. A new ruleLocalSubstituteSurrogateExpressionsis added in the local rewrite batch inLocalLogicalPlanOptimizerto do the substitution.date_trunc/buckettoround_todoes not apply todate_nanosyet, as the underlying APIs support millis only, and nanos has not been tested yet.PreparedRounding.maybeUseArraymay hit an assert if the calendar intervals are multiple quantities, like 10 month or 3 quarter, date histogram aggregation has this limitation documented here,this is likely why the assert hasn't been hit before, and the same limitation will apply to this rewrite.date_trunctoround_tocan be done, however it is not likely to be reflected in the existing perf regression tests. Thenyc_taxisis used by thedate_histogram/date_truncrelated ES|QL tests, it has a wider date range, from year 1900 to 2253, the fixed rounding points of thedatefield -dropoff_datetimecouldn't be calculated for this wide date range, so thedate_truncfunction in the existing perf regressions tests are not rewritten toround_to. With some modifications onnyc_taxis's data - keep data from year 2015 only, the query below shows about ~12% performance improvements, ~840ms(on main) vs ~740ms(on branch wheredate_truncis rewritten toround_to).