Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
ashishjayamohan commented on PR #14385: URL: https://github.com/apache/pinot/pull/14385#issuecomment-2626077604 Sounds good, just let me know when. I'm in PST. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
jadami10 commented on PR #14385: URL: https://github.com/apache/pinot/pull/14385#issuecomment-262516 Yup we can meet. I have work obligations this week, so likely next week. What timezone are you? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
ashishjayamohan commented on PR #14385: URL: https://github.com/apache/pinot/pull/14385#issuecomment-2613741753 @jadami10 Following up on this, sorry for it taking a while! I think there are some implementation-level nuances that I'm running into. Specifically, this optimizer works 100% of the time without timezones but struggles when moving into a different time zone, outputted in a lower granularity unit than the bucketing unit. If you're free and willing, I'd love to schedule a quick call to help walk through some cases since this is my first time working on a feature like this. Let me know if that's a possibility! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
Jackie-Jiang commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1847473141 ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -411,6 +428,80 @@ && isStringLiteral(dateTimeConvertOperands.get(3)), } } + private void optimizeDateTrunc(Function filterFunction, FilterKind filterKind) { +List filterOperands = filterFunction.getOperands(); +List dateTruncOperands = filterOperands.get(0).getFunctionCall().getOperands(); + +// Check if date trunc function is being applied on a literal value +if (dateTruncOperands.get(1).isSetLiteral()) { + return; +} + +Long lowerMillis = null; +Long upperMillis = null; +DateTimeFormatSpec inputFormat = new DateTimeFormatSpec("TIMESTAMP"); +String inputTimeUnit = (dateTruncOperands.size() >= 3) ? dateTruncOperands.get(2).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +String outputTimeUnit = (dateTruncOperands.size() == 5) ? dateTruncOperands.get(4).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +boolean lowerInclusive = true; +boolean upperInclusive = true; +List operands = new ArrayList<>(dateTruncOperands); +switch (filterKind) { + case EQUALS: +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +lowerMillis = dateTruncFloor(operands); +upperMillis = dateTruncCeil(operands); +// Check if it is impossible to obtain literal equality +if (lowerMillis != TimeUnit.valueOf(inputTimeUnit).convert(getLongValue(filterOperands.get(1)), +TimeUnit.valueOf(outputTimeUnit))) { + lowerMillis = Long.MAX_VALUE; + upperMillis = Long.MIN_VALUE; +} +break; + case GREATER_THAN: +lowerInclusive = false; +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +lowerMillis = dateTruncCeil(operands); +break; + case GREATER_THAN_OR_EQUAL: +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +lowerInclusive = false; +lowerMillis = dateTruncCeil(operands); +if (dateTruncFloor(operands) +== inputFormat.fromFormatToMillis(getLongValue(filterOperands.get(1 { + lowerInclusive = true; + lowerMillis = dateTruncFloor(operands); +} +break; + case LESS_THAN: +upperInclusive = false; +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +upperMillis = dateTruncFloor(operands); +if (upperMillis != inputFormat.fromFormatToMillis(getLongValue(filterOperands.get(1 { + upperInclusive = true; + upperMillis = dateTruncCeil(operands); +} +break; + case LESS_THAN_OR_EQUAL: +operands.set(1, filterOperands.get(1)); +upperMillis = dateTruncCeil(operands); +break; + case BETWEEN: +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +operands.set(1, getExpression(filterOperands.get(2), inputFormat, inputTimeUnit, outputTimeUnit)); +upperMillis = dateTruncCeil(operands); +break; + default: +throw new IllegalStateException(); Review Comment: It should be fine because this method is under the branch of `if (filterKind.isRange() || filterKind == FilterKind.EQUALS)` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
jadami10 commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1837438040 ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -411,6 +440,95 @@ && isStringLiteral(dateTimeConvertOperands.get(3)), } } + private void optimizeDateTrunc(Function filterFunction, FilterKind filterKind) { +List filterOperands = filterFunction.getOperands(); +List dateTruncOperands = filterOperands.get(0).getFunctionCall().getOperands(); + +// TODO: Compute value and create query is date trunc is applied on a literal value +if (dateTruncOperands.get(1).isSetLiteral()) { + return; +} + +Long lowerMillis = null; +Long upperMillis = null; +boolean lowerInclusive = true; +boolean upperInclusive = true; +List operands = new ArrayList<>(dateTruncOperands); +String unit = operands.get(0).getLiteral().getStringValue(); +String inputTimeUnit = (operands.size() >= 3) ? operands.get(2).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +ISOChronology chronology = (operands.size() >= 4) +? DateTimeUtils.getChronology(TimeZoneKey.getTimeZoneKey(operands.get(3).getLiteral().getStringValue())) +: ISOChronology.getInstanceUTC(); +String outputTimeUnit = (operands.size() == 5) ? operands.get(4).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +System.out.println(Arrays.toString( +calculateRangeForDateTrunc(unit, getLongValue(filterOperands.get(1)), inputTimeUnit, chronology, +outputTimeUnit))); +switch (filterKind) { + case EQUALS: +operands.set(1, getExpression(getLongValue(filterOperands.get(1)), new DateTimeFormatSpec("TIMESTAMP"))); +upperMillis = dateTruncCeil(operands); +lowerMillis = dateTruncFloor(operands); +if (lowerMillis != TimeUnit.MILLISECONDS.convert(getLongValue(filterOperands.get(1)), TimeUnit.valueOf(outputTimeUnit.toUpperCase( { + lowerMillis = Long.MAX_VALUE; + upperMillis = Long.MIN_VALUE; + String rangeString = new Range(lowerMillis, lowerInclusive, upperMillis, upperInclusive).getRangeString(); + rewriteToRange(filterFunction, dateTruncOperands.get(1), rangeString); + return; +} +break; + case GREATER_THAN: +operands.set(1, getExpression(getLongValue(filterOperands.get(1)), new DateTimeFormatSpec("TIMESTAMP"))); +lowerMillis = dateTruncCeil(operands); +lowerInclusive = false; +upperMillis = Long.MAX_VALUE; +break; + case GREATER_THAN_OR_EQUAL: +operands.set(1, getExpression(getLongValue(filterOperands.get(1)), new DateTimeFormatSpec("TIMESTAMP"))); +lowerMillis = dateTruncFloor(operands); +upperMillis = Long.MAX_VALUE; +if (TimeUnit.valueOf(outputTimeUnit).convert(lowerMillis, TimeUnit.MILLISECONDS) +!= getLongValue(filterOperands.get(1))) { + lowerInclusive = false; + lowerMillis = dateTruncCeil(operands); +} +break; + case LESS_THAN: +operands.set(1, getExpression(getLongValue(filterOperands.get(1)), new DateTimeFormatSpec("TIMESTAMP"))); +lowerMillis = Long.MIN_VALUE; +upperInclusive = false; +upperMillis = dateTruncFloor(operands); +if (upperMillis != TimeUnit.MILLISECONDS.convert(getLongValue(filterOperands.get(1)), TimeUnit.valueOf(outputTimeUnit.toUpperCase( { + upperInclusive = true; + upperMillis = dateTruncCeil(operands); Review Comment: why is this recomputed here? ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -411,6 +440,95 @@ && isStringLiteral(dateTimeConvertOperands.get(3)), } } + private void optimizeDateTrunc(Function filterFunction, FilterKind filterKind) { +List filterOperands = filterFunction.getOperands(); +List dateTruncOperands = filterOperands.get(0).getFunctionCall().getOperands(); + +// TODO: Compute value and create query is date trunc is applied on a literal value +if (dateTruncOperands.get(1).isSetLiteral()) { + return; +} + +Long lowerMillis = null; +Long upperMillis = null; +boolean lowerInclusive = true; +boolean upperInclusive = true; +List operands = new ArrayList<>(dateTruncOperands); +String unit = operands.get(0).getLiteral().getStringValue(); +String inputTimeUnit = (operands.size() >= 3) ? operands.get(2).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +ISOChronology chronology = (operands.size() >= 4) +? DateTimeUtils.getChronology(TimeZoneKey.getTimeZoneKey(operands.get(3).getLiteral().getStringValue())) +: ISOChronology.getInstanceUTC(); +String outputTimeUnit = (operands.size() == 5) ? operands.ge
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
ashishjayamohan commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1836000919 ## pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java: ## @@ -121,38 +114,74 @@ public void testEpochToEpochDateTimeConvert() { new Range(1620833400L, true, null, false)); testTimeConvert("dateTimeConvert(col, '1:MINUTES:EPOCH', '1:HOURS:EPOCH', '30:MINUTES') < 450255", new Range(null, false, 27015300L, false)); -testTimeConvert("dateTimeConvert(col, '1:DAYS:EPOCH', '1:DAYS:EPOCH', '30:MINUTES') BETWEEN 18759 AND 18760", +testTimeConvert("dateTimeConvert(col, '1:DAYS:EPOCH', '1:DAYS:EPOCH', '30:MINUTES') " ++ "BETWEEN 18759 AND 18760", new Range(18759L, true, 18761L, false)); testTimeConvert("dateTimeConvert(col, '1:DAYS:EPOCH', '1:DAYS:EPOCH', '30:MINUTES') = 18759", new Range(18759L, true, 18760L, false)); // Invalid time -testInvalidTimeConvert("dateTimeConvert(col, '1:SECONDS:EPOCH', '1:MINUTES:EPOCH', '30:MINUTES') > 27013846.5"); -testInvalidTimeConvert("dateTimeConvert(col, '1:SECONDS:EPOCH', '30:MINUTES:EPOCH', '30:MINUTES') > 27013846"); +testInvalidFilterOptimizer("dateTimeConvert(col, '1:SECONDS:EPOCH', '1:MINUTES:EPOCH', '30:MINUTES') > 27013846.5"); +testInvalidFilterOptimizer("dateTimeConvert(col, '1:SECONDS:EPOCH', '30:MINUTES:EPOCH', '30:MINUTES') > 27013846"); } @Test public void testSDFToEpochDateTimeConvert() { -testTimeConvert( -"dateTimeConvert(col, '1:MILLISECONDS:SIMPLE_DATE_FORMAT:-MM-dd HH:mm:ss.SSS', '1:MILLISECONDS:EPOCH', " -+ "'30:MINUTES') > 162083076", new Range("2021-05-12 15:00:00.000", true, null, false)); -testTimeConvert("dateTimeConvert(col, '1:SECONDS:SIMPLE_DATE_FORMAT:-MM-dd HH:mm:ss', '1:MILLISECONDS:EPOCH', " -+ "'30:MINUTES') < 162091716", new Range(null, false, "2021-05-13 15:00:00", false)); -testTimeConvert( -"dateTimeConvert(col, '1:MINUTES:SIMPLE_DATE_FORMAT:-MM-dd HH:mm', '1:MILLISECONDS:EPOCH', '30:MINUTES') " -+ "BETWEEN 162083076 AND 162091716", -new Range("2021-05-12 15:00", true, "2021-05-13 15:00", false)); -testTimeConvert( -"dateTimeConvert(col, '1:DAYS:SIMPLE_DATE_FORMAT:-MM-dd', '1:MILLISECONDS:EPOCH', '30:MINUTES') = " -+ "162083076", new Range("2021-05-12", false, "2021-05-12", true)); +testTimeConvert("dateTimeConvert(col, '1:MILLISECONDS:SIMPLE_DATE_FORMAT:-MM-dd HH:mm:ss.SSS', '1:MILLISECONDS:" ++ "EPOCH', '30:MINUTES') > 162083076", new Range("2021-05-12 15:00:00.000", true, null, false)); +testTimeConvert("dateTimeConvert(col, '1:SECONDS:SIMPLE_DATE_FORMAT:-MM-dd HH:mm:ss', '1:MILLISECONDS:EPOCH'," ++ " '30:MINUTES') < 162091716", new Range(null, false, "2021-05-13 15:00:00", false)); +testTimeConvert("dateTimeConvert(col, '1:MINUTES:SIMPLE_DATE_FORMAT:-MM-dd HH:mm', '1:MILLISECONDS:EPOCH', " ++ "'30:MINUTES') BETWEEN 162083076 AND 162091716", new Range("2021-05-12 15:00", true, "2021-05-13 " ++ "15:00", false)); +testTimeConvert("dateTimeConvert(col, '1:DAYS:SIMPLE_DATE_FORMAT:-MM-dd', '1:MILLISECONDS:EPOCH', '30:MINUTES')" ++ " = 162083076", new Range("2021-05-12", false, "2021-05-12", true)); // Invalid time -testInvalidTimeConvert( -"dateTimeConvert(col, '1:MILLISECONDS:SIMPLE_DATE_FORMAT:-MM-dd HH:mm:ss.SSS', '1:MILLISECONDS:EPOCH', " -+ "'30:MINUTES') > 162083076.5"); -testInvalidTimeConvert( -"dateTimeConvert(col, '1:SECONDS:SIMPLE_DATE_FORMAT:-MM-dd HH:mm:ss', '1:MILLISECONDS:EPOCH', " -+ "'30:MINUTES') < 1620917160"); +testInvalidFilterOptimizer("dateTimeConvert(col, '1:MILLISECONDS:SIMPLE_DATE_FORMAT:-MM-dd HH:mm:ss.SSS', " ++ "'1:MILLISECONDS:EPOCH', '30:MINUTES') > 162083076.5"); +testInvalidFilterOptimizer("dateTimeConvert(col, '1:SECONDS:SIMPLE_DATE_FORMAT:-MM-dd HH:mm:ss', " ++ "'1:MILLISECONDS:EPOCH', '30:MINUTES') < 1620917160"); + } + + + @Test + public void testDateTruncOptimizer() { +testDateTrunc("datetrunc('DAY', col) < 162077760", new Range("0", true, "162077760", false)); Review Comment: 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. -- This is an
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
ashishjayamohan commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1833231304 ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -411,6 +428,80 @@ && isStringLiteral(dateTimeConvertOperands.get(3)), } } + private void optimizeDateTrunc(Function filterFunction, FilterKind filterKind) { +List filterOperands = filterFunction.getOperands(); +List dateTruncOperands = filterOperands.get(0).getFunctionCall().getOperands(); + +// Check if date trunc function is being applied on a literal value +if (dateTruncOperands.get(1).isSetLiteral()) { Review Comment: 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
ashishjayamohan commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1833204574 ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -411,6 +428,80 @@ && isStringLiteral(dateTimeConvertOperands.get(3)), } } + private void optimizeDateTrunc(Function filterFunction, FilterKind filterKind) { +List filterOperands = filterFunction.getOperands(); +List dateTruncOperands = filterOperands.get(0).getFunctionCall().getOperands(); + +// Check if date trunc function is being applied on a literal value +if (dateTruncOperands.get(1).isSetLiteral()) { + return; +} + +Long lowerMillis = null; +Long upperMillis = null; +DateTimeFormatSpec inputFormat = new DateTimeFormatSpec("TIMESTAMP"); +String inputTimeUnit = (dateTruncOperands.size() >= 3) ? dateTruncOperands.get(2).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +String outputTimeUnit = (dateTruncOperands.size() == 5) ? dateTruncOperands.get(4).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +boolean lowerInclusive = true; +boolean upperInclusive = true; +List operands = new ArrayList<>(dateTruncOperands); +switch (filterKind) { + case EQUALS: +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +lowerMillis = dateTruncFloor(operands); +upperMillis = dateTruncCeil(operands); +// Check if it is impossible to obtain literal equality +if (lowerMillis != TimeUnit.valueOf(inputTimeUnit).convert(getLongValue(filterOperands.get(1)), +TimeUnit.valueOf(outputTimeUnit))) { + lowerMillis = Long.MAX_VALUE; + upperMillis = Long.MIN_VALUE; +} +break; + case GREATER_THAN: +lowerInclusive = false; +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +lowerMillis = dateTruncCeil(operands); +break; + case GREATER_THAN_OR_EQUAL: +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +lowerInclusive = false; +lowerMillis = dateTruncCeil(operands); +if (dateTruncFloor(operands) +== inputFormat.fromFormatToMillis(getLongValue(filterOperands.get(1 { + lowerInclusive = true; + lowerMillis = dateTruncFloor(operands); +} +break; + case LESS_THAN: +upperInclusive = false; +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +upperMillis = dateTruncFloor(operands); +if (upperMillis != inputFormat.fromFormatToMillis(getLongValue(filterOperands.get(1 { + upperInclusive = true; + upperMillis = dateTruncCeil(operands); +} +break; + case LESS_THAN_OR_EQUAL: +operands.set(1, filterOperands.get(1)); +upperMillis = dateTruncCeil(operands); +break; + case BETWEEN: +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +operands.set(1, getExpression(filterOperands.get(2), inputFormat, inputTimeUnit, outputTimeUnit)); +upperMillis = dateTruncCeil(operands); +break; + default: +throw new IllegalStateException(); Review Comment: This is in following with the standard in the other optimization functions -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
ashishjayamohan commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1833156423 ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -411,6 +428,80 @@ && isStringLiteral(dateTimeConvertOperands.get(3)), } } + private void optimizeDateTrunc(Function filterFunction, FilterKind filterKind) { +List filterOperands = filterFunction.getOperands(); +List dateTruncOperands = filterOperands.get(0).getFunctionCall().getOperands(); + +// Check if date trunc function is being applied on a literal value +if (dateTruncOperands.get(1).isSetLiteral()) { + return; +} + +Long lowerMillis = null; +Long upperMillis = null; +DateTimeFormatSpec inputFormat = new DateTimeFormatSpec("TIMESTAMP"); +String inputTimeUnit = (dateTruncOperands.size() >= 3) ? dateTruncOperands.get(2).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +String outputTimeUnit = (dateTruncOperands.size() == 5) ? dateTruncOperands.get(4).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +boolean lowerInclusive = true; +boolean upperInclusive = true; +List operands = new ArrayList<>(dateTruncOperands); +switch (filterKind) { + case EQUALS: +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +lowerMillis = dateTruncFloor(operands); +upperMillis = dateTruncCeil(operands); Review Comment: See: https://github.com/apache/pinot/pull/14385#discussion_r1833152007 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
ashishjayamohan commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1833152007 ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -411,6 +428,80 @@ && isStringLiteral(dateTimeConvertOperands.get(3)), } } + private void optimizeDateTrunc(Function filterFunction, FilterKind filterKind) { +List filterOperands = filterFunction.getOperands(); +List dateTruncOperands = filterOperands.get(0).getFunctionCall().getOperands(); + +// Check if date trunc function is being applied on a literal value +if (dateTruncOperands.get(1).isSetLiteral()) { + return; +} + +Long lowerMillis = null; +Long upperMillis = null; +DateTimeFormatSpec inputFormat = new DateTimeFormatSpec("TIMESTAMP"); +String inputTimeUnit = (dateTruncOperands.size() >= 3) ? dateTruncOperands.get(2).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +String outputTimeUnit = (dateTruncOperands.size() == 5) ? dateTruncOperands.get(4).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +boolean lowerInclusive = true; +boolean upperInclusive = true; +List operands = new ArrayList<>(dateTruncOperands); +switch (filterKind) { + case EQUALS: +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +lowerMillis = dateTruncFloor(operands); +upperMillis = dateTruncCeil(operands); +// Check if it is impossible to obtain literal equality +if (lowerMillis != TimeUnit.valueOf(inputTimeUnit).convert(getLongValue(filterOperands.get(1)), Review Comment: Good catch! I actually made a mistake here. This line should be: `if (lowerMillis != getLongValue(filterOperands.get(1),` Essentially, in equality predicates it is possible that the literal value (`filterOperands.get(1)`) is not a feasible output from `dateTrunc`. In this case, there are no values that could be truncated to that literal. Consider this example where a specified literal is not on an even granularity step (assume that the granularity is the . In this case, an equality predicate would be existentially false. We can check this by taking the regular `dateTrunc` of the literal and checking if the output is equal to the literal itself.  In the case where the specified literal *is* on an even granularity step, like below, we have multiple acceptable values. Specifically, the range from the literal to the next granularity step (non-inclusive of the maximum). This case is identified when the `dateTrunc` of the literal equals the literal.  -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
ashishjayamohan commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1832110522 ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -411,6 +428,80 @@ && isStringLiteral(dateTimeConvertOperands.get(3)), } } + private void optimizeDateTrunc(Function filterFunction, FilterKind filterKind) { +List filterOperands = filterFunction.getOperands(); +List dateTruncOperands = filterOperands.get(0).getFunctionCall().getOperands(); + +// Check if date trunc function is being applied on a literal value +if (dateTruncOperands.get(1).isSetLiteral()) { + return; +} + +Long lowerMillis = null; +Long upperMillis = null; +DateTimeFormatSpec inputFormat = new DateTimeFormatSpec("TIMESTAMP"); +String inputTimeUnit = (dateTruncOperands.size() >= 3) ? dateTruncOperands.get(2).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); Review Comment: Yep, default is milliseconds. https://docs.pinot.apache.org/configuration-reference/functions/datetrunc -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
jadami10 commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1831979437 ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -411,6 +428,80 @@ && isStringLiteral(dateTimeConvertOperands.get(3)), } } + private void optimizeDateTrunc(Function filterFunction, FilterKind filterKind) { +List filterOperands = filterFunction.getOperands(); +List dateTruncOperands = filterOperands.get(0).getFunctionCall().getOperands(); + +// Check if date trunc function is being applied on a literal value +if (dateTruncOperands.get(1).isSetLiteral()) { + return; +} + +Long lowerMillis = null; +Long upperMillis = null; +DateTimeFormatSpec inputFormat = new DateTimeFormatSpec("TIMESTAMP"); +String inputTimeUnit = (dateTruncOperands.size() >= 3) ? dateTruncOperands.get(2).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); Review Comment: is the default milliseconds if you just do `DATETRUNC(unit, timeValue)`? ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -411,6 +428,80 @@ && isStringLiteral(dateTimeConvertOperands.get(3)), } } + private void optimizeDateTrunc(Function filterFunction, FilterKind filterKind) { +List filterOperands = filterFunction.getOperands(); +List dateTruncOperands = filterOperands.get(0).getFunctionCall().getOperands(); + +// Check if date trunc function is being applied on a literal value +if (dateTruncOperands.get(1).isSetLiteral()) { + return; +} + +Long lowerMillis = null; +Long upperMillis = null; +DateTimeFormatSpec inputFormat = new DateTimeFormatSpec("TIMESTAMP"); +String inputTimeUnit = (dateTruncOperands.size() >= 3) ? dateTruncOperands.get(2).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +String outputTimeUnit = (dateTruncOperands.size() == 5) ? dateTruncOperands.get(4).getLiteral().getStringValue() +: TimeUnit.MILLISECONDS.name(); +boolean lowerInclusive = true; +boolean upperInclusive = true; +List operands = new ArrayList<>(dateTruncOperands); +switch (filterKind) { + case EQUALS: +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +lowerMillis = dateTruncFloor(operands); +upperMillis = dateTruncCeil(operands); +// Check if it is impossible to obtain literal equality +if (lowerMillis != TimeUnit.valueOf(inputTimeUnit).convert(getLongValue(filterOperands.get(1)), +TimeUnit.valueOf(outputTimeUnit))) { + lowerMillis = Long.MAX_VALUE; + upperMillis = Long.MIN_VALUE; +} +break; + case GREATER_THAN: +lowerInclusive = false; +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +lowerMillis = dateTruncCeil(operands); +break; + case GREATER_THAN_OR_EQUAL: +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +lowerInclusive = false; +lowerMillis = dateTruncCeil(operands); +if (dateTruncFloor(operands) +== inputFormat.fromFormatToMillis(getLongValue(filterOperands.get(1 { + lowerInclusive = true; + lowerMillis = dateTruncFloor(operands); +} +break; + case LESS_THAN: +upperInclusive = false; +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +upperMillis = dateTruncFloor(operands); +if (upperMillis != inputFormat.fromFormatToMillis(getLongValue(filterOperands.get(1 { + upperInclusive = true; + upperMillis = dateTruncCeil(operands); +} +break; + case LESS_THAN_OR_EQUAL: +operands.set(1, filterOperands.get(1)); +upperMillis = dateTruncCeil(operands); +break; + case BETWEEN: +operands.set(1, getExpression(filterOperands.get(1), inputFormat, inputTimeUnit, outputTimeUnit)); +operands.set(1, getExpression(filterOperands.get(2), inputFormat, inputTimeUnit, outputTimeUnit)); +upperMillis = dateTruncCeil(operands); +break; + default: +throw new IllegalStateException(); Review Comment: shouldn't we just return here? ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -411,6 +428,80 @@ && isStringLiteral(dateTimeConvertOperands.get(3)), } } + private void optimizeDateTrunc(Function filterFunction, FilterKind filterKind) { +List filterOperands = filterFunction.getOperands(); +List dateTruncOperands = filterOperands.get(0).getFunctionCall().getOperands(); + +// Check i
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
Jackie-Jiang commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1831641424 ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -84,6 +88,8 @@ Expression optimize(Expression filterExpression) { optimizeTimeConvert(filterFunction, filterKind); } else if (functionName.equalsIgnoreCase(DateTimeConversionTransformFunction.FUNCTION_NAME)) { optimizeDateTimeConvert(filterFunction, filterKind); +} else if (functionName.equalsIgnoreCase(DateTruncTransformFunction.FUNCTION_NAME)) { Review Comment: Please also update the javadoc of this class for this new support -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
Jackie-Jiang commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1831640321 ## pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java: ## @@ -50,15 +50,18 @@ public void testTimeConvert() { new Range(162077760L, true, 162086400L, false)); // Other input format -testTimeConvert("timeConvert(col, 'MINUTES', 'SECONDS') > 1620830760", new Range(27013846L, false, null, false)); -testTimeConvert("timeConvert(col, 'HOURS', 'MINUTES') < 27015286", new Range(null, false, 450254L, true)); +testTimeConvert("timeConvert(col, 'MINUTES', 'SECONDS') > 1620830760", Review Comment: (format) There are some wrong auto-format. Can you double check your formatter setting? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
ashishjayamohan commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1830325783 ## pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java: ## @@ -411,6 +417,96 @@ && isStringLiteral(dateTimeConvertOperands.get(3)), } } + private void optimizeDateTrunc(Function filterFunction, FilterKind filterKind) { +List filterOperands = filterFunction.getOperands(); +List dateTruncOperands = filterOperands.get(0).getFunctionCall().getOperands(); + +// Check if date trunc function is being applied on a literal value +if (dateTruncOperands.get(1).isSetLiteral()) { + return; +} + +Long lowerMillis = null; +Long upperMillis = null; +DateTimeFormatSpec inputFormat = (dateTruncOperands.size() >= 3) +? new DateTimeFormatSpec(dateTruncOperands.get(2).getLiteral().getStringValue()) +: new DateTimeFormatSpec("TIMESTAMP"); +boolean lowerInclusive = true; +boolean upperInclusive = true; +List operands = new ArrayList<>(dateTruncOperands); +switch (filterKind) { + case EQUALS: +operands.set(1, filterOperands.get(1)); +lowerMillis = dateTruncFloor(operands); +upperMillis = dateTruncCeil(operands); +// Check if it is impossible to obtain literal equality +if (lowerMillis != inputFormat.fromFormatToMillis(filterOperands.get(1).getLiteral().getLongValue())) { + lowerMillis = Long.MAX_VALUE; + upperMillis = Long.MIN_VALUE; +} +break; + case GREATER_THAN: +lowerInclusive = false; +operands.set(1, filterOperands.get(1)); +lowerMillis = dateTruncCeil(operands); +break; + case GREATER_THAN_OR_EQUAL: +operands.set(1, filterOperands.get(1)); +lowerInclusive = false; +lowerMillis = dateTruncCeil(operands); +if (dateTruncFloor(operands) +== inputFormat.fromFormatToMillis(filterOperands.get(1).getLiteral().getLongValue())) { + lowerInclusive = true; + lowerMillis = dateTruncFloor(operands); +} +break; + case LESS_THAN: +upperInclusive = false; +operands.set(1, filterOperands.get(1)); +upperMillis = dateTruncFloor(operands); +if (upperMillis != inputFormat.fromFormatToMillis(filterOperands.get(1).getLiteral().getLongValue())) { + upperInclusive = true; + upperMillis = dateTruncCeil(operands); +} +break; + case LESS_THAN_OR_EQUAL: +operands.set(1, filterOperands.get(1)); +upperMillis = dateTruncCeil(operands); +break; + case BETWEEN: +Literal lowerLiteral = new Literal(); +if (filterOperands.get(1).getLiteral().isSetStringValue()) { + lowerLiteral.setLongValue(new DateTimeFormatSpec("TIMESTAMP").fromFormatToMillis( + filterOperands.get(1).getLiteral().getStringValue())); +} else { + lowerLiteral.setLongValue(getLongValue(filterOperands.get(1))); +} +Expression lowerExpression = new Expression(ExpressionType.LITERAL); +lowerExpression.setLiteral(lowerLiteral); +operands.set(1, lowerExpression); +lowerMillis = dateTruncCeil(operands); +Literal upperLiteral = new Literal(); +if (filterOperands.get(2).getLiteral().isSetStringValue()) { + upperLiteral.setLongValue(new DateTimeFormatSpec("TIMESTAMP").fromFormatToMillis( Review Comment: Make output format with TIMESTAMP as default, output format if specified -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
ashishjayamohan commented on code in PR #14385: URL: https://github.com/apache/pinot/pull/14385#discussion_r1830324905 ## pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java: ## @@ -147,14 +155,65 @@ public void testSDFToEpochDateTimeConvert() { + "162083076", new Range("2021-05-12", false, "2021-05-12", true)); // Invalid time -testInvalidTimeConvert( +testInvalidFilterOptimizer( "dateTimeConvert(col, '1:MILLISECONDS:SIMPLE_DATE_FORMAT:-MM-dd HH:mm:ss.SSS', '1:MILLISECONDS:EPOCH', " + "'30:MINUTES') > 162083076.5"); -testInvalidTimeConvert( +testInvalidFilterOptimizer( "dateTimeConvert(col, '1:SECONDS:SIMPLE_DATE_FORMAT:-MM-dd HH:mm:ss', '1:MILLISECONDS:EPOCH', " + "'30:MINUTES') < 1620917160"); } + + @Test + public void testDateTruncOptimizer() { +testDateTrunc( +"datetrunc('DAY', col) < 162077760", new Range("0", true, "162077760", false)); Review Comment: Need to add tests covering the other invocations of dateTrunc -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add `DATE_TRUNC` Optimizer [pinot]
ashishjayamohan commented on PR #14385: URL: https://github.com/apache/pinot/pull/14385#issuecomment-2458496986 @jadami10 Made that optimizer enhancement here. Let me know if anything looks off! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org