Add `DATE_TRUNC` Optimizer #14385

ashishjayamohan · 2024-11-05T05:59:16Z

Closes: Time Pruner should support queries where we're operating on the time column itself #14362
Addresses: Short circuit queries where no rows will be returned #13563
Adds optimizer for dateTrunc function

codecov-commenter · 2024-11-05T07:02:58Z

Codecov Report

Attention: Patch coverage is 0% with 102 lines in your changes missing coverage. Please review.

Project coverage is 34.12%. Comparing base (59551e4) to head (2e6bb78).
Report is 1308 commits behind head on master.

Files with missing lines	Patch %	Lines
...optimizer/filter/TimePredicateFilterOptimizer.java	0.00%	101 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (59551e4) and HEAD (2e6bb78). Click for more details.

HEAD has 13 uploads less than BASE

Flag BASE (59551e4) HEAD (2e6bb78)

integration2 3 1

temurin 12 10

skip-bytebuffers-false 7 4

unittests 5 3

unittests1 2 0

java-11 5 3

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #14385       +/-   ##
=============================================
- Coverage     61.75%   34.12%   -27.63%     
- Complexity      207      778      +571     
=============================================
  Files          2436     2661      +225     
  Lines        133233   146117    +12884     
  Branches      20636    22379     +1743     
=============================================
- Hits          82274    49866    -32408     
- Misses        44911    92223    +47312     
+ Partials       6048     4028     -2020

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`34.12% <0.00%> (-27.59%)`	⬇️
java-21	`34.12% <0.00%> (-27.51%)`	⬇️
skip-bytebuffers-false	`34.12% <0.00%> (-27.63%)`	⬇️
skip-bytebuffers-true	`34.12% <0.00%> (+6.39%)`	⬆️
temurin	`34.12% <0.00%> (-27.63%)`	⬇️
unittests	`34.12% <0.00%> (-27.63%)`	⬇️
unittests1	`?`
unittests2	`34.12% <0.00%> (+6.39%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ashishjayamohan · 2024-11-06T01:02:12Z

@jadami10 Made that optimizer enhancement here. Let me know if anything looks off!

...test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java

...src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java

...test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java

...src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java

jadami10

first, thank you for doing this!

took an initial look and left some nit comments, some testing ideas, and some ideas about stuff that looks like it might break.

I haven't reviewed the actual algorithm yet

jadami10 · 2024-11-07T02:47:33Z

...src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java

+    // Check if date trunc function is being applied on a literal value
+    if (dateTruncOperands.get(1).isSetLiteral()) {


I think you want a testInvalidFilterOptimizer unit test for this.

nit: this comment is identical to the code below it. a better comment would say why we can't/don't optimize literal values.

Since DATETRUNC can be applied on literals, my thinking was that if we returned at this point, some other optimizer would precompute this value. I need to look into the other optimizers to figure out if this is the case. If this is not applied elsewhere, I'll add the computation for the DATETRUNC of the literal and new query creation here.

...src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java

...test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java

jadami10 · 2024-11-07T02:58:56Z

...test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java

+
+  @Test
+  public void testDateTruncOptimizer() {
+    testDateTrunc("datetrunc('DAY', col) < 1620777600000", new Range("0", true, "1620777600000", false));


do we need a test with an INT instead of long? I believe that's also a valid time type in pinot

can we have test with week or month truncation

can we have a test case where it's not a support function. as in IN to make sure nothing is erroring

also a test case where the time granularity is unsupported? I'm not sure if calcite will catch that before the optimizer, but we do use DAY as an input

Hey @jadami10. Thanks for this insight! I've been working on this for the past couple days (specifically on the time zone test). I've introduced several time zone tests and my implementation seems to work only for some time zone usages.
If you're willing, would you be able to write up a quick draft of what the algorithm should look like to convert the date_trunc function with time zones to a range query (essentially, the floor and ceiling inverse of date trunc). I think it would be beneficial to hear it from another perspective to find what I'm missing.

jadami10

i think your approach to convert the date trunc predicates to floor/ceiling sounds good. I left some comments where I'm confused about why the implementation drifts for similar functions

...src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java

jadami10 · 2024-11-12T03:38:53Z

...src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java

+        upperMillis = dateTruncFloor(operands);
+        if (upperMillis != TimeUnit.MILLISECONDS.convert(getLongValue(filterOperands.get(1)), TimeUnit.valueOf(outputTimeUnit.toUpperCase()))) {
+          upperInclusive = true;
+          upperMillis = dateTruncCeil(operands);


why is this recomputed here?

jadami10 · 2024-11-12T03:40:41Z

...src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java

+        lowerMillis = Long.MIN_VALUE;
+        upperInclusive = false;
+        upperMillis = dateTruncFloor(operands);
+        if (upperMillis != TimeUnit.MILLISECONDS.convert(getLongValue(filterOperands.get(1)), TimeUnit.valueOf(outputTimeUnit.toUpperCase()))) {


why are we checking this here but not in in GREATER_THAN?

jadami10 · 2024-11-12T03:42:55Z

...src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java

+        break;
+      case GREATER_THAN_OR_EQUAL:
+        operands.set(1, getExpression(getLongValue(filterOperands.get(1)), new DateTimeFormatSpec("TIMESTAMP")));
+        lowerMillis = dateTruncFloor(operands);


should this be ceil?

jadami10 · 2024-11-12T03:45:06Z

...test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java

+//    testDateTrunc("datetrunc('DAY', col, 'DAYS', 'CET', 'MILLISECONDS') = 39193714800000",
+//        new Range("453631", true, "453631", true));
+    testDateTrunc("datetrunc('DAY', col, 'MILLISECONDS', 'UTC', 'DAYS') = 453630",
+        new Range("39193632000000", true, "39193718399999", true));


this looks strange. 39193632000000 is like 1000 years from now?

ashishjayamohan and others added 5 commits November 4, 2024 14:38

add floor and ceil func

e3efa65

Merge branch 'apache:master' into optimize-date-trunc

731b943

add date trunc optimization

749bbbb

fix style

c2dbe08

fix range query column expr

8dda15e

ashishjayamohan added 8 commits November 5, 2024 11:20

add support for between

2edad2a

refactor test functions

52490a3

refactor back and add test

cb387da

add tests for range pred

0b41d25

style

9391b48

fix algo and tests

36e7639

fix between operator

bc08f03

add tests

b09fc49

ashishjayamohan changed the title ~~[draft] Optimize DATETRUNC in Predicate~~ Add DATE_TRUNC Optimizer Nov 6, 2024

ashishjayamohan marked this pull request as ready for review November 6, 2024 00:59

ashishjayamohan commented Nov 6, 2024

View reviewed changes

...test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java Outdated Show resolved Hide resolved

ashishjayamohan commented Nov 6, 2024

View reviewed changes

...src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java Outdated Show resolved Hide resolved

ashishjayamohan added 4 commits November 5, 2024 22:01

use correct output format

eec0e7f

add helper function

614e176

make unit step less brittle

624e360

correct conversion to output unit

5e87ed7

Jackie-Jiang added the enhancement label Nov 6, 2024

Jackie-Jiang reviewed Nov 6, 2024

View reviewed changes

...test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java Show resolved Hide resolved

Jackie-Jiang reviewed Nov 6, 2024

View reviewed changes

...src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java Show resolved Hide resolved

add javadocs

4b836c7

jadami10 reviewed Nov 7, 2024

View reviewed changes

ashishjayamohan added 2 commits November 7, 2024 12:46

fix short circuit

470d4bb

fix range settings

321cc6a

everything but time zones

e3a6651

ashishjayamohan marked this pull request as draft November 12, 2024 00:43

just logging for future

2e6bb78

jadami10 reviewed Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `DATE_TRUNC` Optimizer #14385

Add `DATE_TRUNC` Optimizer #14385

ashishjayamohan commented Nov 5, 2024 •

edited

Loading

codecov-commenter commented Nov 5, 2024 •

edited

Loading

ashishjayamohan commented Nov 6, 2024

jadami10 left a comment •

edited

Loading

jadami10 Nov 7, 2024

ashishjayamohan Nov 7, 2024

jadami10 Nov 7, 2024

ashishjayamohan Nov 11, 2024

jadami10 left a comment

jadami10 Nov 12, 2024

jadami10 Nov 12, 2024

jadami10 Nov 12, 2024

jadami10 Nov 12, 2024

		// Check if date trunc function is being applied on a literal value
		if (dateTruncOperands.get(1).isSetLiteral()) {

Add DATE_TRUNC Optimizer #14385

Are you sure you want to change the base?

Add DATE_TRUNC Optimizer #14385

Conversation

ashishjayamohan commented Nov 5, 2024 • edited Loading

codecov-commenter commented Nov 5, 2024 • edited Loading

Codecov Report

ashishjayamohan commented Nov 6, 2024

jadami10 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadami10 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `DATE_TRUNC` Optimizer #14385

Add `DATE_TRUNC` Optimizer #14385

ashishjayamohan commented Nov 5, 2024 •

edited

Loading

codecov-commenter commented Nov 5, 2024 •

edited

Loading

jadami10 left a comment •

edited

Loading