You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/05/21 05:40:36 UTC
[GitHub] [incubator-pinot] Jackie-Jiang opened a new pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Jackie-Jiang opened a new pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957
## Description
Solve #6910
Add `TimePredicateFilterOptimizer` to optimize TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate to the inner expression.
E.g. `dateTimeConvert(col, '1:MILLISECONDS:EPOCH', '1:MILLISECONDS:EPOCH', '30:MINUTES') >= 1620830760000` can be optimized to `col >= 1620831600000`
After optimizing the time convert function to directly work on the time column, the pruner and all the indexes can be applied.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637165379
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java
##########
@@ -59,11 +62,13 @@ public void optimize(BrokerRequest brokerRequest, @Nullable Schema schema) {
*/
public void optimize(PinotQuery pinotQuery, @Nullable Schema schema) {
Expression filterExpression = pinotQuery.getFilterExpression();
- if (filterExpression != null) {
- for (FilterOptimizer filterOptimizer : FILTER_OPTIMIZERS) {
+ for (FilterOptimizer filterOptimizer : FILTER_OPTIMIZERS) {
+ if (filterExpression != null && filterExpression.getType() == ExpressionType.FUNCTION) {
Review comment:
+1
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637252312
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import java.util.List;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class TimePredicateFilterOptimizerTest {
+ private static final TimePredicateFilterOptimizer OPTIMIZER = new TimePredicateFilterOptimizer();
+
+ @Test
+ public void testTimeConvert() {
+ testTimeConvert("timeConvert(col, 'MILLISECONDS', 'MILLISECONDS') > 1620830760000",
+ new Range(1620830760001L, true, null, false));
Review comment:
Also it seems like, `timconvert(col, 'MILLISECONDS', 'MILLISECONDS') > 9223372036854775807` (long max value) will turn into `col >= 9223372036854775807` which seems incorrect. A test case on this would be useful.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637207805
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
+ Expression expression = operands.get(0);
+ if (expression.getType() == ExpressionType.FUNCTION) {
+ Function expressionFunction = expression.getFunctionCall();
+ String functionName = StringUtils.remove(expressionFunction.getOperator(), '_');
+ if (functionName.equalsIgnoreCase(TimeConversionTransformFunction.FUNCTION_NAME)) {
+ optimizeTimeConvert(filterFunction, filterKind);
+ } else if (functionName.equalsIgnoreCase(DateTimeConversionTransformFunction.FUNCTION_NAME)) {
+ optimizeDateTimeConvert(filterFunction, filterKind);
+ }
+ }
+ }
+ return filterExpression;
+ }
+
+ /**
+ * Helper method to optimize TIME_CONVERT function with range/equality predicate to directly apply the predicate to
+ * the inner expression. Changes are applied in-place of the filter function.
+ */
+ private void optimizeTimeConvert(Function filterFunction, FilterKind filterKind) {
+ List<Expression> filterOperands = filterFunction.getOperands();
+ List<Expression> timeConvertOperands = filterOperands.get(0).getFunctionCall().getOperands();
+ Preconditions.checkArgument(timeConvertOperands.size() == 3,
+ "Exactly 3 arguments are required for TIME_CONVERT transform function");
+ Preconditions
+ .checkArgument(isStringLiteral(timeConvertOperands.get(1)) && isStringLiteral(timeConvertOperands.get(2)),
+ "The 2nd and 3rd argument for TIME_CONVERT transform function must be string literal");
+
+ try {
+ TimeUnit inputTimeUnit = TimeUnit.valueOf(timeConvertOperands.get(1).getLiteral().getStringValue().toUpperCase());
+ TimeUnit outputTimeUnit =
+ TimeUnit.valueOf(timeConvertOperands.get(2).getLiteral().getStringValue().toUpperCase());
+
+ // Step 1: Convert output range to millis range
+ Long lowerMillis = null;
+ Long upperMillis = null;
+ switch (filterKind) {
+ case GREATER_THAN: {
+ // millisToFormat(millis) > n
+ // -> millisToFormat(millis) >= n + 1
+ // -> millis >= formatToMillis(n + 1)
+ long lowerValue = Long.parseLong(filterOperands.get(1).getLiteral().getFieldValue().toString());
+ lowerMillis = outputTimeUnit.toMillis(lowerValue + 1);
+ break;
+ }
+ case GREATER_THAN_OR_EQUAL: {
+ // millisToFormat(millis) >= n
+ // -> millis >= formatToMillis(n)
+ long lowerValue = Long.parseLong(filterOperands.get(1).getLiteral().getFieldValue().toString());
+ lowerMillis = outputTimeUnit.toMillis(lowerValue);
+ break;
+ }
+ case LESS_THAN: {
+ // millisToFormat(millis) < n
+ // -> millis < formatToMillis(n)
+ long upperValue = Long.parseLong(filterOperands.get(1).getLiteral().getFieldValue().toString());
+ upperMillis = outputTimeUnit.toMillis(upperValue);
+ break;
+ }
+ case LESS_THAN_OR_EQUAL: {
+ // millisToFormat(millis) <= n
+ // -> millisToFormat(millis) < n + 1
+ // -> millis < formatToMillis(n + 1)
+ long upperValue = Long.parseLong(filterOperands.get(1).getLiteral().getFieldValue().toString());
+ upperMillis = outputTimeUnit.toMillis(upperValue + 1);
+ break;
+ }
+ case BETWEEN: {
+ // Combine GREATER_THAN_OR_EQUAL and LESS_THAN_OR_EQUAL
+ long lowerValue = Long.parseLong(filterOperands.get(1).getLiteral().getFieldValue().toString());
+ lowerMillis = outputTimeUnit.toMillis(lowerValue);
+ long upperValue = Long.parseLong(filterOperands.get(2).getLiteral().getFieldValue().toString());
+ upperMillis = outputTimeUnit.toMillis(upperValue + 1);
+ break;
+ }
+ case EQUALS: {
+ // Combine GREATER_THAN_OR_EQUAL and LESS_THAN_OR_EQUAL
+ long value = Long.parseLong(filterOperands.get(1).getLiteral().getFieldValue().toString());
+ lowerMillis = outputTimeUnit.toMillis(value);
+ upperMillis = outputTimeUnit.toMillis(value + 1);
+ break;
+ }
+ default:
+ throw new IllegalStateException();
+ }
+
+ // Step 2: Convert millis range to input range
+ Long lowerValue = null;
+ boolean lowerInclusive = false;
+ if (lowerMillis != null) {
+ // formatToMillis(col) >= millis
+ // - if (formatToMillis(millisToFormat(millis)) == millis)
+ // -> col >= millisToFormat(millis)
+ // - else (formatToMillis(millisToFormat(millis)) < millis)
+ // -> col > millisToFormat(millis)
+ lowerValue = inputTimeUnit.convert(lowerMillis, TimeUnit.MILLISECONDS);
+ lowerInclusive = inputTimeUnit.toMillis(lowerValue) == lowerMillis;
+ }
+ Long upperValue = null;
+ boolean upperInclusive = false;
+ if (upperMillis != null) {
+ // formatToMillis(col) < millis
+ // - if (formatToMillis(millisToFormat(millis)) == millis)
+ // -> col < millisToFormat(millis)
+ // - else (formatToMillis(millisToFormat(millis)) < millis)
+ // -> col <= millisToFormat(millis)
+ upperValue = inputTimeUnit.convert(upperMillis, TimeUnit.MILLISECONDS);
+ upperInclusive = inputTimeUnit.toMillis(upperValue) != upperMillis;
+ }
+
+ // Step 3: Rewrite the filter function
+ String rangeString = new Range(lowerValue, lowerInclusive, upperValue, upperInclusive).getRangeString();
+ filterFunction.setOperator(FilterKind.RANGE.name());
+ filterFunction
+ .setOperands(Arrays.asList(timeConvertOperands.get(0), RequestUtils.getLiteralExpression(rangeString)));
+ } catch (Exception e) {
+ LOGGER.warn("Caught exception while optimizing TIME_CONVERT predicate: {}, skipping the optimization",
+ filterFunction, e);
+ }
+ }
+
+ /**
+ * Helper method to optimize DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression. Changes are applied in-place of the filter function.
+ */
+ private void optimizeDateTimeConvert(Function filterFunction, FilterKind filterKind) {
+ List<Expression> filterOperands = filterFunction.getOperands();
+ List<Expression> dateTimeConvertOperands = filterOperands.get(0).getFunctionCall().getOperands();
+ Preconditions.checkArgument(dateTimeConvertOperands.size() == 4,
+ "Exactly 4 arguments are required for DATE_TIME_CONVERT transform function");
+ Preconditions.checkArgument(
+ isStringLiteral(dateTimeConvertOperands.get(1)) && isStringLiteral(dateTimeConvertOperands.get(2))
+ && isStringLiteral(dateTimeConvertOperands.get(3)),
+ "The 2nd to 4th arguments for DATE_TIME_CONVERT transform function must be string literal");
+
+ try {
+ DateTimeFormatSpec inputFormat =
+ new DateTimeFormatSpec(dateTimeConvertOperands.get(1).getLiteral().getStringValue());
+ DateTimeFormatSpec outputFormat =
+ new DateTimeFormatSpec(dateTimeConvertOperands.get(2).getLiteral().getStringValue());
+ // SDF output format is not supported because:
+ // 1. No easy way to get the next time value (instead of simply +1 for EPOCH format)
+ // 2. Hard to calculate the bucket boundary (need to consider time zone)
+ // TODO: Support SDF output format
+ if (outputFormat.getTimeFormat() == TimeFormat.SIMPLE_DATE_FORMAT) {
+ return;
+ }
+ long granularityMillis = new DateTimeGranularitySpec(dateTimeConvertOperands.get(3).getLiteral().getStringValue())
+ .granularityToMillis();
+
+ // Step 1: Convert output range to millis range
Review comment:
I considered using the same code for both `time_convert` and `date_time_convert`, but that would introduce unnecessary overhead for `time_convert` because it does not require SDF and bucketing support. Basically it's a trade-off between performance and code complexity
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637312512
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
Review comment:
Done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637208782
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
+ Expression expression = operands.get(0);
+ if (expression.getType() == ExpressionType.FUNCTION) {
+ Function expressionFunction = expression.getFunctionCall();
+ String functionName = StringUtils.remove(expressionFunction.getOperator(), '_');
+ if (functionName.equalsIgnoreCase(TimeConversionTransformFunction.FUNCTION_NAME)) {
+ optimizeTimeConvert(filterFunction, filterKind);
+ } else if (functionName.equalsIgnoreCase(DateTimeConversionTransformFunction.FUNCTION_NAME)) {
+ optimizeDateTimeConvert(filterFunction, filterKind);
+ }
+ }
+ }
+ return filterExpression;
+ }
+
+ /**
+ * Helper method to optimize TIME_CONVERT function with range/equality predicate to directly apply the predicate to
+ * the inner expression. Changes are applied in-place of the filter function.
+ */
+ private void optimizeTimeConvert(Function filterFunction, FilterKind filterKind) {
+ List<Expression> filterOperands = filterFunction.getOperands();
+ List<Expression> timeConvertOperands = filterOperands.get(0).getFunctionCall().getOperands();
+ Preconditions.checkArgument(timeConvertOperands.size() == 3,
+ "Exactly 3 arguments are required for TIME_CONVERT transform function");
+ Preconditions
+ .checkArgument(isStringLiteral(timeConvertOperands.get(1)) && isStringLiteral(timeConvertOperands.get(2)),
+ "The 2nd and 3rd argument for TIME_CONVERT transform function must be string literal");
+
+ try {
+ TimeUnit inputTimeUnit = TimeUnit.valueOf(timeConvertOperands.get(1).getLiteral().getStringValue().toUpperCase());
+ TimeUnit outputTimeUnit =
+ TimeUnit.valueOf(timeConvertOperands.get(2).getLiteral().getStringValue().toUpperCase());
+
+ // Step 1: Convert output range to millis range
+ Long lowerMillis = null;
+ Long upperMillis = null;
+ switch (filterKind) {
+ case GREATER_THAN: {
+ // millisToFormat(millis) > n
+ // -> millisToFormat(millis) >= n + 1
+ // -> millis >= formatToMillis(n + 1)
Review comment:
We have to do `+1` here, or it won't be correct. E.g. `millisToSeconds(millis) > 0` is not equivalent to `millis > 0` because of the flooring of the value. Will add the example into the comment
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637157988
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/MergeRangeFilterOptimizer.java
##########
@@ -114,26 +112,6 @@ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schem
}
}
- /**
Review comment:
Extracted into a separate class so that it can be used by other optimizers
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637169202
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
Review comment:
Are we not rewriting for `NOT_EQUALS, IN, NOT_IN`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-commenter commented on pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#issuecomment-845689733
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6957](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a988b2e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/16f94a19a82807244617e7436c7ba57a1ed56164?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (16f94a1) will **decrease** coverage by `7.91%`.
> The diff coverage is `81.48%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6957 +/- ##
============================================
- Coverage 73.30% 65.39% -7.92%
Complexity 12 12
============================================
Files 1432 1434 +2
Lines 70970 71100 +130
Branches 10288 10308 +20
============================================
- Hits 52025 46496 -5529
- Misses 15469 21249 +5780
+ Partials 3476 3355 -121
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `?` | `?` | |
| unittests | `65.39% <81.48%> (+0.03%)` | `12.00 <0.00> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...apache/pinot/spi/data/DateTimeGranularitySpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUdyYW51bGFyaXR5U3BlYy5qYXZh) | `86.66% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [...ry/optimizer/filter/MergeRangeFilterOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL01lcmdlUmFuZ2VGaWx0ZXJPcHRpbWl6ZXIuamF2YQ==) | `87.96% <33.33%> (-6.38%)` | `0.00 <0.00> (ø)` | |
| [...optimizer/filter/TimePredicateFilterOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL1RpbWVQcmVkaWNhdGVGaWx0ZXJPcHRpbWl6ZXIuamF2YQ==) | `78.46% <78.46%> (ø)` | `0.00 <0.00> (?)` | |
| [...pinot/core/query/optimizer/filter/utils/Range.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL3V0aWxzL1JhbmdlLmphdmE=) | `90.19% <90.19%> (ø)` | `0.00 <0.00> (?)` | |
| [...che/pinot/core/query/optimizer/QueryOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvUXVlcnlPcHRpbWl6ZXIuamF2YQ==) | `100.00% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...ery/optimizer/filter/MergeEqInFilterOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL01lcmdlRXFJbkZpbHRlck9wdGltaXplci5qYXZh) | `95.07% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| ... and [345 more](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [16f94a1...a988b2e](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637159224
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/utils/Range.java
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter.utils;
Review comment:
This is a helper class for the filter optimizers. Moved it one level up to `org.apache.pinot.core.query.optimizer.filter`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637200232
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
+ Expression expression = operands.get(0);
+ if (expression.getType() == ExpressionType.FUNCTION) {
+ Function expressionFunction = expression.getFunctionCall();
+ String functionName = StringUtils.remove(expressionFunction.getOperator(), '_');
+ if (functionName.equalsIgnoreCase(TimeConversionTransformFunction.FUNCTION_NAME)) {
+ optimizeTimeConvert(filterFunction, filterKind);
+ } else if (functionName.equalsIgnoreCase(DateTimeConversionTransformFunction.FUNCTION_NAME)) {
+ optimizeDateTimeConvert(filterFunction, filterKind);
+ }
+ }
+ }
+ return filterExpression;
+ }
+
+ /**
+ * Helper method to optimize TIME_CONVERT function with range/equality predicate to directly apply the predicate to
+ * the inner expression. Changes are applied in-place of the filter function.
+ */
+ private void optimizeTimeConvert(Function filterFunction, FilterKind filterKind) {
+ List<Expression> filterOperands = filterFunction.getOperands();
+ List<Expression> timeConvertOperands = filterOperands.get(0).getFunctionCall().getOperands();
+ Preconditions.checkArgument(timeConvertOperands.size() == 3,
+ "Exactly 3 arguments are required for TIME_CONVERT transform function");
+ Preconditions
+ .checkArgument(isStringLiteral(timeConvertOperands.get(1)) && isStringLiteral(timeConvertOperands.get(2)),
+ "The 2nd and 3rd argument for TIME_CONVERT transform function must be string literal");
+
+ try {
+ TimeUnit inputTimeUnit = TimeUnit.valueOf(timeConvertOperands.get(1).getLiteral().getStringValue().toUpperCase());
+ TimeUnit outputTimeUnit =
+ TimeUnit.valueOf(timeConvertOperands.get(2).getLiteral().getStringValue().toUpperCase());
+
+ // Step 1: Convert output range to millis range
+ Long lowerMillis = null;
+ Long upperMillis = null;
+ switch (filterKind) {
+ case GREATER_THAN: {
+ // millisToFormat(millis) > n
+ // -> millisToFormat(millis) >= n + 1
+ // -> millis >= formatToMillis(n + 1)
+ long lowerValue = Long.parseLong(filterOperands.get(1).getLiteral().getFieldValue().toString());
+ lowerMillis = outputTimeUnit.toMillis(lowerValue + 1);
+ break;
+ }
+ case GREATER_THAN_OR_EQUAL: {
+ // millisToFormat(millis) >= n
+ // -> millis >= formatToMillis(n)
+ long lowerValue = Long.parseLong(filterOperands.get(1).getLiteral().getFieldValue().toString());
+ lowerMillis = outputTimeUnit.toMillis(lowerValue);
+ break;
+ }
+ case LESS_THAN: {
+ // millisToFormat(millis) < n
+ // -> millis < formatToMillis(n)
+ long upperValue = Long.parseLong(filterOperands.get(1).getLiteral().getFieldValue().toString());
+ upperMillis = outputTimeUnit.toMillis(upperValue);
+ break;
+ }
+ case LESS_THAN_OR_EQUAL: {
+ // millisToFormat(millis) <= n
+ // -> millisToFormat(millis) < n + 1
+ // -> millis < formatToMillis(n + 1)
+ long upperValue = Long.parseLong(filterOperands.get(1).getLiteral().getFieldValue().toString());
+ upperMillis = outputTimeUnit.toMillis(upperValue + 1);
+ break;
+ }
+ case BETWEEN: {
+ // Combine GREATER_THAN_OR_EQUAL and LESS_THAN_OR_EQUAL
+ long lowerValue = Long.parseLong(filterOperands.get(1).getLiteral().getFieldValue().toString());
+ lowerMillis = outputTimeUnit.toMillis(lowerValue);
+ long upperValue = Long.parseLong(filterOperands.get(2).getLiteral().getFieldValue().toString());
+ upperMillis = outputTimeUnit.toMillis(upperValue + 1);
+ break;
+ }
+ case EQUALS: {
+ // Combine GREATER_THAN_OR_EQUAL and LESS_THAN_OR_EQUAL
+ long value = Long.parseLong(filterOperands.get(1).getLiteral().getFieldValue().toString());
+ lowerMillis = outputTimeUnit.toMillis(value);
+ upperMillis = outputTimeUnit.toMillis(value + 1);
+ break;
+ }
+ default:
+ throw new IllegalStateException();
+ }
+
+ // Step 2: Convert millis range to input range
+ Long lowerValue = null;
+ boolean lowerInclusive = false;
+ if (lowerMillis != null) {
+ // formatToMillis(col) >= millis
+ // - if (formatToMillis(millisToFormat(millis)) == millis)
+ // -> col >= millisToFormat(millis)
+ // - else (formatToMillis(millisToFormat(millis)) < millis)
+ // -> col > millisToFormat(millis)
+ lowerValue = inputTimeUnit.convert(lowerMillis, TimeUnit.MILLISECONDS);
+ lowerInclusive = inputTimeUnit.toMillis(lowerValue) == lowerMillis;
+ }
+ Long upperValue = null;
+ boolean upperInclusive = false;
+ if (upperMillis != null) {
+ // formatToMillis(col) < millis
+ // - if (formatToMillis(millisToFormat(millis)) == millis)
+ // -> col < millisToFormat(millis)
+ // - else (formatToMillis(millisToFormat(millis)) < millis)
+ // -> col <= millisToFormat(millis)
+ upperValue = inputTimeUnit.convert(upperMillis, TimeUnit.MILLISECONDS);
+ upperInclusive = inputTimeUnit.toMillis(upperValue) != upperMillis;
+ }
+
+ // Step 3: Rewrite the filter function
+ String rangeString = new Range(lowerValue, lowerInclusive, upperValue, upperInclusive).getRangeString();
+ filterFunction.setOperator(FilterKind.RANGE.name());
+ filterFunction
+ .setOperands(Arrays.asList(timeConvertOperands.get(0), RequestUtils.getLiteralExpression(rangeString)));
+ } catch (Exception e) {
+ LOGGER.warn("Caught exception while optimizing TIME_CONVERT predicate: {}, skipping the optimization",
+ filterFunction, e);
+ }
+ }
+
+ /**
+ * Helper method to optimize DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression. Changes are applied in-place of the filter function.
+ */
+ private void optimizeDateTimeConvert(Function filterFunction, FilterKind filterKind) {
+ List<Expression> filterOperands = filterFunction.getOperands();
+ List<Expression> dateTimeConvertOperands = filterOperands.get(0).getFunctionCall().getOperands();
+ Preconditions.checkArgument(dateTimeConvertOperands.size() == 4,
+ "Exactly 4 arguments are required for DATE_TIME_CONVERT transform function");
+ Preconditions.checkArgument(
+ isStringLiteral(dateTimeConvertOperands.get(1)) && isStringLiteral(dateTimeConvertOperands.get(2))
+ && isStringLiteral(dateTimeConvertOperands.get(3)),
+ "The 2nd to 4th arguments for DATE_TIME_CONVERT transform function must be string literal");
+
+ try {
+ DateTimeFormatSpec inputFormat =
+ new DateTimeFormatSpec(dateTimeConvertOperands.get(1).getLiteral().getStringValue());
+ DateTimeFormatSpec outputFormat =
+ new DateTimeFormatSpec(dateTimeConvertOperands.get(2).getLiteral().getStringValue());
+ // SDF output format is not supported because:
+ // 1. No easy way to get the next time value (instead of simply +1 for EPOCH format)
+ // 2. Hard to calculate the bucket boundary (need to consider time zone)
+ // TODO: Support SDF output format
+ if (outputFormat.getTimeFormat() == TimeFormat.SIMPLE_DATE_FORMAT) {
+ return;
+ }
+ long granularityMillis = new DateTimeGranularitySpec(dateTimeConvertOperands.get(3).getLiteral().getStringValue())
+ .granularityToMillis();
+
+ // Step 1: Convert output range to millis range
Review comment:
Can step 1 and step 2 code be refactored and made common between `optimizeTimeConvert `and `optimizeDateTimeConvert` 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#issuecomment-845689733
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6957](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (713bb40) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/4d834eae6916e198a42ae4221e126d548dc26fe7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4d834ea) will **decrease** coverage by `7.96%`.
> The diff coverage is `83.41%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6957 +/- ##
============================================
- Coverage 73.35% 65.39% -7.97%
Complexity 12 12
============================================
Files 1432 1434 +2
Lines 70986 71137 +151
Branches 10288 10310 +22
============================================
- Hits 52074 46518 -5556
- Misses 15432 21261 +5829
+ Partials 3480 3358 -122
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `?` | `?` | |
| unittests | `65.39% <83.41%> (+0.02%)` | `12.00 <0.00> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...apache/pinot/spi/data/DateTimeGranularitySpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUdyYW51bGFyaXR5U3BlYy5qYXZh) | `86.66% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [...ry/optimizer/filter/MergeRangeFilterOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL01lcmdlUmFuZ2VGaWx0ZXJPcHRpbWl6ZXIuamF2YQ==) | `88.88% <50.00%> (-5.46%)` | `0.00 <0.00> (ø)` | |
| [...optimizer/filter/TimePredicateFilterOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL1RpbWVQcmVkaWNhdGVGaWx0ZXJPcHRpbWl6ZXIuamF2YQ==) | `81.45% <81.45%> (ø)` | `0.00 <0.00> (?)` | |
| [...pache/pinot/core/query/optimizer/filter/Range.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL1JhbmdlLmphdmE=) | `90.19% <90.19%> (ø)` | `0.00 <0.00> (?)` | |
| [...che/pinot/core/query/optimizer/QueryOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvUXVlcnlPcHRpbWl6ZXIuamF2YQ==) | `100.00% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| ... and [342 more](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4d834ea...713bb40](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637207324
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import java.util.List;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class TimePredicateFilterOptimizerTest {
+ private static final TimePredicateFilterOptimizer OPTIMIZER = new TimePredicateFilterOptimizer();
+
+ @Test
+ public void testTimeConvert() {
+ testTimeConvert("timeConvert(col, 'MILLISECONDS', 'MILLISECONDS') > 1620830760000",
+ new Range(1620830760001L, true, null, false));
Review comment:
Yes, but my point is that we shouldn't be changing input values unless absolutely needed. In this case the +1 can be easily avoided.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637164795
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
+ Expression expression = operands.get(0);
+ if (expression.getType() == ExpressionType.FUNCTION) {
+ Function expressionFunction = expression.getFunctionCall();
+ String functionName = StringUtils.remove(expressionFunction.getOperator(), '_');
Review comment:
We ignore the underscore within the function name so that both `dateConvert` and `date_convert` are supported. Currently we don't have a central place to canonicalize the function names, so we have to remove the underscore 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637165572
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
+ Expression expression = operands.get(0);
+ if (expression.getType() == ExpressionType.FUNCTION) {
+ Function expressionFunction = expression.getFunctionCall();
+ String functionName = StringUtils.remove(expressionFunction.getOperator(), '_');
+ if (functionName.equalsIgnoreCase(TimeConversionTransformFunction.FUNCTION_NAME)) {
+ optimizeTimeConvert(filterFunction, filterKind);
+ } else if (functionName.equalsIgnoreCase(DateTimeConversionTransformFunction.FUNCTION_NAME)) {
+ optimizeDateTimeConvert(filterFunction, filterKind);
+ }
+ }
+ }
+ return filterExpression;
+ }
+
+ /**
+ * Helper method to optimize TIME_CONVERT function with range/equality predicate to directly apply the predicate to
+ * the inner expression. Changes are applied in-place of the filter function.
+ */
+ private void optimizeTimeConvert(Function filterFunction, FilterKind filterKind) {
+ List<Expression> filterOperands = filterFunction.getOperands();
+ List<Expression> timeConvertOperands = filterOperands.get(0).getFunctionCall().getOperands();
+ Preconditions.checkArgument(timeConvertOperands.size() == 3,
Review comment:
Currently the argument check is performed on the server side, but the optimization is performed before routing the query. We can consider moving the argument check to broker to block bad query, but that is out of the scope of this PR
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637167958
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
Review comment:
@sajjad-moradi For this optimizer we can do that because all the changes are applied in-place. Will add a note and change it
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637153642
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java
##########
@@ -59,11 +62,13 @@ public void optimize(BrokerRequest brokerRequest, @Nullable Schema schema) {
*/
public void optimize(PinotQuery pinotQuery, @Nullable Schema schema) {
Expression filterExpression = pinotQuery.getFilterExpression();
- if (filterExpression != null) {
- for (FilterOptimizer filterOptimizer : FILTER_OPTIMIZERS) {
+ for (FilterOptimizer filterOptimizer : FILTER_OPTIMIZERS) {
+ if (filterExpression != null && filterExpression.getType() == ExpressionType.FUNCTION) {
Review comment:
Currently the filter expression can only be function or boolean literal, so the intention here is to short-circuit the extra filter optimizer if the filter is already optimized to a boolean literal.
While this is not closely related to the scope of this PR, and in the future we might want to support column/udf as the filter, so reverting this part to keep it flexible.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#issuecomment-845689733
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6957](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (713bb40) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/4d834eae6916e198a42ae4221e126d548dc26fe7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4d834ea) will **decrease** coverage by `0.05%`.
> The diff coverage is `85.36%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6957 +/- ##
============================================
- Coverage 73.35% 73.30% -0.06%
Complexity 12 12
============================================
Files 1432 1434 +2
Lines 70986 71137 +151
Branches 10288 10310 +22
============================================
+ Hits 52074 52147 +73
- Misses 15432 15497 +65
- Partials 3480 3493 +13
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `42.88% <48.78%> (-0.03%)` | `7.00 <0.00> (ø)` | |
| unittests | `65.39% <83.41%> (+0.02%)` | `12.00 <0.00> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...apache/pinot/spi/data/DateTimeGranularitySpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUdyYW51bGFyaXR5U3BlYy5qYXZh) | `86.66% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [...ry/optimizer/filter/MergeRangeFilterOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL01lcmdlUmFuZ2VGaWx0ZXJPcHRpbWl6ZXIuamF2YQ==) | `92.59% <50.00%> (-1.75%)` | `0.00 <0.00> (ø)` | |
| [...optimizer/filter/TimePredicateFilterOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL1RpbWVQcmVkaWNhdGVGaWx0ZXJPcHRpbWl6ZXIuamF2YQ==) | `82.11% <82.11%> (ø)` | `0.00 <0.00> (?)` | |
| [...pache/pinot/core/query/optimizer/filter/Range.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL1JhbmdlLmphdmE=) | `96.07% <96.07%> (ø)` | `0.00 <0.00> (?)` | |
| [...che/pinot/core/query/optimizer/QueryOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvUXVlcnlPcHRpbWl6ZXIuamF2YQ==) | `100.00% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...data/manager/realtime/SegmentCommitterFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU2VnbWVudENvbW1pdHRlckZhY3RvcnkuamF2YQ==) | `64.70% <0.00%> (-35.30%)` | `0.00% <0.00%> (ø%)` | |
| [...mpl/dictionary/DoubleOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT2ZmSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `57.44% <0.00%> (-11.71%)` | `0.00% <0.00%> (ø%)` | |
| [...altime/ServerSegmentCompletionProtocolHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VydmVyL3JlYWx0aW1lL1NlcnZlclNlZ21lbnRDb21wbGV0aW9uUHJvdG9jb2xIYW5kbGVyLmphdmE=) | `48.54% <0.00%> (-8.74%)` | `0.00% <0.00%> (ø%)` | |
| [...er/api/resources/LLCSegmentCompletionHandlers.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL0xMQ1NlZ21lbnRDb21wbGV0aW9uSGFuZGxlcnMuamF2YQ==) | `51.16% <0.00%> (-8.38%)` | `0.00% <0.00%> (ø%)` | |
| [...e/data/manager/realtime/SplitSegmentCommitter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU3BsaXRTZWdtZW50Q29tbWl0dGVyLmphdmE=) | `53.84% <0.00%> (-7.70%)` | `0.00% <0.00%> (ø%)` | |
| ... and [22 more](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4d834ea...713bb40](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#issuecomment-845689733
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6957](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1374a1c) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/4d834eae6916e198a42ae4221e126d548dc26fe7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4d834ea) will **decrease** coverage by `7.94%`.
> The diff coverage is `83.41%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6957 +/- ##
============================================
- Coverage 73.35% 65.41% -7.95%
Complexity 12 12
============================================
Files 1432 1434 +2
Lines 70986 71137 +151
Branches 10288 10310 +22
============================================
- Hits 52074 46536 -5538
- Misses 15432 21249 +5817
+ Partials 3480 3352 -128
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `?` | `?` | |
| unittests | `65.41% <83.41%> (+0.04%)` | `12.00 <0.00> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...apache/pinot/spi/data/DateTimeGranularitySpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUdyYW51bGFyaXR5U3BlYy5qYXZh) | `86.66% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [...ry/optimizer/filter/MergeRangeFilterOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL01lcmdlUmFuZ2VGaWx0ZXJPcHRpbWl6ZXIuamF2YQ==) | `88.88% <50.00%> (-5.46%)` | `0.00 <0.00> (ø)` | |
| [...optimizer/filter/TimePredicateFilterOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL1RpbWVQcmVkaWNhdGVGaWx0ZXJPcHRpbWl6ZXIuamF2YQ==) | `81.45% <81.45%> (ø)` | `0.00 <0.00> (?)` | |
| [...pache/pinot/core/query/optimizer/filter/Range.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL1JhbmdlLmphdmE=) | `90.19% <90.19%> (ø)` | `0.00 <0.00> (?)` | |
| [...che/pinot/core/query/optimizer/QueryOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvUXVlcnlPcHRpbWl6ZXIuamF2YQ==) | `100.00% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| ... and [342 more](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4d834ea...1374a1c](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637252312
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import java.util.List;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class TimePredicateFilterOptimizerTest {
+ private static final TimePredicateFilterOptimizer OPTIMIZER = new TimePredicateFilterOptimizer();
+
+ @Test
+ public void testTimeConvert() {
+ testTimeConvert("timeConvert(col, 'MILLISECONDS', 'MILLISECONDS') > 1620830760000",
+ new Range(1620830760001L, true, null, false));
Review comment:
Also this is an edge case, but it seems like, `timeconvert(col, 'MILLISECONDS', 'MILLISECONDS') > 9223372036854775807` (long max value) will turn into `col >= 9223372036854775807` which seems incorrect. A test case on this would be useful.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r636652258
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/MergeEqInFilterOptimizer.java
##########
@@ -151,7 +150,7 @@ private static FilterQueryTree getFilterQueryTree(String column, Set<String> val
@Override
public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
- return filterExpression.getType() == ExpressionType.FUNCTION ? optimize(filterExpression) : filterExpression;
+ return optimize(filterExpression);
Review comment:
revert this
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/MergeRangeFilterOptimizer.java
##########
@@ -114,26 +112,6 @@ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schem
}
}
- /**
Review comment:
why was this removed?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/utils/Range.java
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter.utils;
Review comment:
why under utils?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java
##########
@@ -59,11 +62,13 @@ public void optimize(BrokerRequest brokerRequest, @Nullable Schema schema) {
*/
public void optimize(PinotQuery pinotQuery, @Nullable Schema schema) {
Expression filterExpression = pinotQuery.getFilterExpression();
- if (filterExpression != null) {
- for (FilterOptimizer filterOptimizer : FILTER_OPTIMIZERS) {
+ for (FilterOptimizer filterOptimizer : FILTER_OPTIMIZERS) {
+ if (filterExpression != null && filterExpression.getType() == ExpressionType.FUNCTION) {
Review comment:
I see that you moved the logic from each optimizer which was checking if its a function one level up.. while its a good optimization, I don't think its a good design, lets say we add another optimizer that does not care, then someone has to change all the existing optimizers.
My suggestion is to let each optimizer decide if it can optimize the filterExpression or have a method on the optimizer to check if its only applicable to function expression
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
+ Expression expression = operands.get(0);
+ if (expression.getType() == ExpressionType.FUNCTION) {
+ Function expressionFunction = expression.getFunctionCall();
+ String functionName = StringUtils.remove(expressionFunction.getOperator(), '_');
+ if (functionName.equalsIgnoreCase(TimeConversionTransformFunction.FUNCTION_NAME)) {
+ optimizeTimeConvert(filterFunction, filterKind);
+ } else if (functionName.equalsIgnoreCase(DateTimeConversionTransformFunction.FUNCTION_NAME)) {
+ optimizeDateTimeConvert(filterFunction, filterKind);
+ }
+ }
+ }
+ return filterExpression;
+ }
+
+ /**
+ * Helper method to optimize TIME_CONVERT function with range/equality predicate to directly apply the predicate to
+ * the inner expression. Changes are applied in-place of the filter function.
+ */
+ private void optimizeTimeConvert(Function filterFunction, FilterKind filterKind) {
+ List<Expression> filterOperands = filterFunction.getOperands();
+ List<Expression> timeConvertOperands = filterOperands.get(0).getFunctionCall().getOperands();
+ Preconditions.checkArgument(timeConvertOperands.size() == 3,
Review comment:
this should be done somewhere else right?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java
##########
@@ -59,11 +62,13 @@ public void optimize(BrokerRequest brokerRequest, @Nullable Schema schema) {
*/
public void optimize(PinotQuery pinotQuery, @Nullable Schema schema) {
Expression filterExpression = pinotQuery.getFilterExpression();
- if (filterExpression != null) {
- for (FilterOptimizer filterOptimizer : FILTER_OPTIMIZERS) {
+ for (FilterOptimizer filterOptimizer : FILTER_OPTIMIZERS) {
+ if (filterExpression != null && filterExpression.getType() == ExpressionType.FUNCTION) {
Review comment:
this is changing the existing behavior right? looks like it optimizes only the filterExpression is a function?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
Review comment:
can you add some examples?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java
##########
@@ -32,12 +33,14 @@
import org.apache.pinot.core.query.optimizer.filter.MergeEqInFilterOptimizer;
import org.apache.pinot.core.query.optimizer.filter.MergeRangeFilterOptimizer;
import org.apache.pinot.core.query.optimizer.filter.NumericalFilterOptimizer;
+import org.apache.pinot.core.query.optimizer.filter.TimePredicateFilterOptimizer;
import org.apache.pinot.spi.data.Schema;
public class QueryOptimizer {
private static final List<FilterOptimizer> FILTER_OPTIMIZERS = Arrays
- .asList(new FlattenAndOrFilterOptimizer(), new NumericalFilterOptimizer(), new MergeEqInFilterOptimizer(), new MergeRangeFilterOptimizer());
+ .asList(new FlattenAndOrFilterOptimizer(), new MergeEqInFilterOptimizer(), new NumericalFilterOptimizer(),
Review comment:
does the order matter?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
+ Expression expression = operands.get(0);
+ if (expression.getType() == ExpressionType.FUNCTION) {
+ Function expressionFunction = expression.getFunctionCall();
+ String functionName = StringUtils.remove(expressionFunction.getOperator(), '_');
Review comment:
why?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
Review comment:
when did we start using this coding convention?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637319302
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
Review comment:
Added some note in the javadoc of the class explaining why it is not supported
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
Review comment:
Added some note in the javadoc of the class explaining why it is not supported
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#issuecomment-845689733
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#6957](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f34d853) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/4d834eae6916e198a42ae4221e126d548dc26fe7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4d834ea) will **decrease** coverage by `30.45%`.
> The diff coverage is `51.70%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #6957 +/- ##
=============================================
- Coverage 73.35% 42.90% -30.46%
+ Complexity 12 7 -5
=============================================
Files 1432 1434 +2
Lines 70986 71137 +151
Branches 10288 10310 +22
=============================================
- Hits 52074 30522 -21552
- Misses 15432 37980 +22548
+ Partials 3480 2635 -845
```
| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| integration | `42.90% <51.70%> (+<0.01%)` | `7.00 <0.00> (ø)` | |
| unittests | `?` | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...apache/pinot/spi/data/DateTimeGranularitySpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUdyYW51bGFyaXR5U3BlYy5qYXZh) | `0.00% <ø> (-86.67%)` | `0.00 <0.00> (ø)` | |
| [...optimizer/filter/TimePredicateFilterOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL1RpbWVQcmVkaWNhdGVGaWx0ZXJPcHRpbWl6ZXIuamF2YQ==) | `36.42% <36.42%> (ø)` | `0.00 <0.00> (?)` | |
| [...ry/optimizer/filter/MergeRangeFilterOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL01lcmdlUmFuZ2VGaWx0ZXJPcHRpbWl6ZXIuamF2YQ==) | `90.74% <50.00%> (-3.60%)` | `0.00 <0.00> (ø)` | |
| [...pache/pinot/core/query/optimizer/filter/Range.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvZmlsdGVyL1JhbmdlLmphdmE=) | `96.07% <96.07%> (ø)` | `0.00 <0.00> (?)` | |
| [...che/pinot/core/query/optimizer/QueryOptimizer.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9vcHRpbWl6ZXIvUXVlcnlPcHRpbWl6ZXIuamF2YQ==) | `100.00% <100.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...a/org/apache/pinot/spi/data/DateTimeFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EYXRlVGltZUZpZWxkU3BlYy5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| [...a/org/apache/pinot/spi/env/PinotConfiguration.java](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZW52L1Bpbm90Q29uZmlndXJhdGlvbi5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
| ... and [908 more](https://codecov.io/gh/apache/incubator-pinot/pull/6957/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4d834ea...f34d853](https://codecov.io/gh/apache/incubator-pinot/pull/6957?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang merged pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637145939
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java
##########
@@ -32,12 +33,14 @@
import org.apache.pinot.core.query.optimizer.filter.MergeEqInFilterOptimizer;
import org.apache.pinot.core.query.optimizer.filter.MergeRangeFilterOptimizer;
import org.apache.pinot.core.query.optimizer.filter.NumericalFilterOptimizer;
+import org.apache.pinot.core.query.optimizer.filter.TimePredicateFilterOptimizer;
import org.apache.pinot.spi.data.Schema;
public class QueryOptimizer {
private static final List<FilterOptimizer> FILTER_OPTIMIZERS = Arrays
- .asList(new FlattenAndOrFilterOptimizer(), new NumericalFilterOptimizer(), new MergeEqInFilterOptimizer(), new MergeRangeFilterOptimizer());
+ .asList(new FlattenAndOrFilterOptimizer(), new MergeEqInFilterOptimizer(), new NumericalFilterOptimizer(),
Review comment:
Yes, the order matters. Will add some comments 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637205635
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import java.util.List;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class TimePredicateFilterOptimizerTest {
+ private static final TimePredicateFilterOptimizer OPTIMIZER = new TimePredicateFilterOptimizer();
+
+ @Test
+ public void testTimeConvert() {
+ testTimeConvert("timeConvert(col, 'MILLISECONDS', 'MILLISECONDS') > 1620830760000",
+ new Range(1620830760001L, true, null, false));
+ testTimeConvert("TIME_CONVERT(col, 'MILLISECONDS', 'MILLISECONDS') < 1620917160000",
+ new Range(null, false, 1620917160000L, false));
+ testTimeConvert("timeconvert(col, 'MILLISECONDS', 'MILLISECONDS') BETWEEN 1620830760000 AND 1620917160000",
+ new Range(1620830760000L, true, 1620917160001L, false));
Review comment:
I think we should avoid changing input values unless absolutely needed. In this case the Range should be `new Range(1620830760000L, true, 162091716000L, true))`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637294027
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import java.util.List;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class TimePredicateFilterOptimizerTest {
+ private static final TimePredicateFilterOptimizer OPTIMIZER = new TimePredicateFilterOptimizer();
+
+ @Test
+ public void testTimeConvert() {
+ testTimeConvert("timeConvert(col, 'MILLISECONDS', 'MILLISECONDS') > 1620830760000",
+ new Range(1620830760001L, true, null, false));
Review comment:
I think it would be much simpler and accurate if you don't do +1. That way it will handle all cases and you won't need to add a if statement for large values.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637164166
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
Review comment:
How about NOT_EQUALS filter?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637206251
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import java.util.List;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class TimePredicateFilterOptimizerTest {
+ private static final TimePredicateFilterOptimizer OPTIMIZER = new TimePredicateFilterOptimizer();
+
+ @Test
+ public void testTimeConvert() {
+ testTimeConvert("timeConvert(col, 'MILLISECONDS', 'MILLISECONDS') > 1620830760000",
+ new Range(1620830760001L, true, null, false));
Review comment:
These 2 ranges are equivalent
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637252312
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import java.util.List;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class TimePredicateFilterOptimizerTest {
+ private static final TimePredicateFilterOptimizer OPTIMIZER = new TimePredicateFilterOptimizer();
+
+ @Test
+ public void testTimeConvert() {
+ testTimeConvert("timeConvert(col, 'MILLISECONDS', 'MILLISECONDS') > 1620830760000",
+ new Range(1620830760001L, true, null, false));
Review comment:
Also it seems like, `timeconvert(col, 'MILLISECONDS', 'MILLISECONDS') > 9223372036854775807` (long max value) will turn into `col >= 9223372036854775807` which seems incorrect. A test case on this would be useful.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637475860
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import java.util.List;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class TimePredicateFilterOptimizerTest {
+ private static final TimePredicateFilterOptimizer OPTIMIZER = new TimePredicateFilterOptimizer();
+
+ @Test
+ public void testTimeConvert() {
+ testTimeConvert("timeConvert(col, 'MILLISECONDS', 'MILLISECONDS') > 1620830760000",
+ new Range(1620830760001L, true, null, false));
Review comment:
I see, 2071-1-1 is the current max timestamp. I guess I can live with that. Although it seems like all the semantic checks should be done outside and before the optimizers are run (but that could be refactored at a later time).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637165227
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java
##########
@@ -32,12 +33,14 @@
import org.apache.pinot.core.query.optimizer.filter.MergeEqInFilterOptimizer;
import org.apache.pinot.core.query.optimizer.filter.MergeRangeFilterOptimizer;
import org.apache.pinot.core.query.optimizer.filter.NumericalFilterOptimizer;
+import org.apache.pinot.core.query.optimizer.filter.TimePredicateFilterOptimizer;
import org.apache.pinot.spi.data.Schema;
public class QueryOptimizer {
private static final List<FilterOptimizer> FILTER_OPTIMIZERS = Arrays
- .asList(new FlattenAndOrFilterOptimizer(), new NumericalFilterOptimizer(), new MergeEqInFilterOptimizer(), new MergeRangeFilterOptimizer());
+ .asList(new FlattenAndOrFilterOptimizer(), new MergeEqInFilterOptimizer(), new NumericalFilterOptimizer(),
Review comment:
We should add comments here to describe dependencies between optimizers.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637161711
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
Review comment:
This should be faster because it avoids the per element `get()` and `set()` and the redundant boundary check. We want to avoid the stream API, which might cause performance issue
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637169202
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
Review comment:
Are we not rewriting for `NOT_EQUALS, IN, NOT_IN`? If not, a comment to explain why would be useful.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#issuecomment-846513840
@kishoreg @sajjad-moradi @amrishlal Thanks for the review. Addressed the comments and added example queries in the PR description. For more query examples, check the `TimePredicateFilterOptimizerTest`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637201149
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import java.util.List;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class TimePredicateFilterOptimizerTest {
+ private static final TimePredicateFilterOptimizer OPTIMIZER = new TimePredicateFilterOptimizer();
+
+ @Test
+ public void testTimeConvert() {
+ testTimeConvert("timeConvert(col, 'MILLISECONDS', 'MILLISECONDS') > 1620830760000",
+ new Range(1620830760001L, true, null, false));
Review comment:
I think this should made to be `new Range(1620830760000L, false, null, false))`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637161568
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
Review comment:
why replaceAll? You can do `operands.forEach(this::optimize)` instead. Here you want to apply optimize function on all operands. Although operand.replaceAll does the work, but it's a bit confusing and there's no need to replace the list elements.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637282223
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import java.util.List;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class TimePredicateFilterOptimizerTest {
+ private static final TimePredicateFilterOptimizer OPTIMIZER = new TimePredicateFilterOptimizer();
+
+ @Test
+ public void testTimeConvert() {
+ testTimeConvert("timeConvert(col, 'MILLISECONDS', 'MILLISECONDS') > 1620830760000",
+ new Range(1620830760001L, true, null, false));
+ testTimeConvert("TIME_CONVERT(col, 'MILLISECONDS', 'MILLISECONDS') < 1620917160000",
+ new Range(null, false, 1620917160000L, false));
+ testTimeConvert("timeconvert(col, 'MILLISECONDS', 'MILLISECONDS') BETWEEN 1620830760000 AND 1620917160000",
+ new Range(1620830760000L, true, 1620917160001L, false));
Review comment:
Special case the same input output time unit case to directly remove the `TIME_CONVERT` transform. Although it should be super rare because it is basically no-op.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637317462
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import java.util.List;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class TimePredicateFilterOptimizerTest {
+ private static final TimePredicateFilterOptimizer OPTIMIZER = new TimePredicateFilterOptimizer();
+
+ @Test
+ public void testTimeConvert() {
+ testTimeConvert("timeConvert(col, 'MILLISECONDS', 'MILLISECONDS') > 1620830760000",
+ new Range(1620830760001L, true, null, false));
Review comment:
We can avoid the `+1` only when the output format is `MILLISECONDS`, and it will complicate the step 2 (convert millis range to input range). Since they are equivalent, I'd prefer just keeping it simple. Added a check that millis must be in valid range to avoid unexpected overflow/underflow.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] amrishlal commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
amrishlal commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637197543
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizer.java
##########
@@ -0,0 +1,330 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.core.operator.transform.function.DateTimeConversionTransformFunction;
+import org.apache.pinot.core.operator.transform.function.TimeConversionTransformFunction;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.spi.data.DateTimeFieldSpec.TimeFormat;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimePredicateFilterOptimizer} optimizes the time related predicates:
+ * <ul>
+ * <li>
+ * Optimizes TIME_CONVERT/DATE_TIME_CONVERT function with range/equality predicate to directly apply the predicate
+ * to the inner expression.
+ * </li>
+ * </ul>
+ *
+ * NOTE: This optimizer is followed by the {@link MergeRangeFilterOptimizer}, which can merge the generated ranges.
+ */
+public class TimePredicateFilterOptimizer implements FilterOptimizer {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TimePredicateFilterOptimizer.class);
+
+ @Override
+ public FilterQueryTree optimize(FilterQueryTree filterQueryTree, @Nullable Schema schema) {
+ // Do not rewrite PQL queries because PQL is deprecated
+ return filterQueryTree;
+ }
+
+ @Override
+ public Expression optimize(Expression filterExpression, @Nullable Schema schema) {
+ return optimize(filterExpression);
+ }
+
+ @VisibleForTesting
+ Expression optimize(Expression filterExpression) {
+ Function filterFunction = filterExpression.getFunctionCall();
+ FilterKind filterKind = FilterKind.valueOf(filterFunction.getOperator());
+ List<Expression> operands = filterFunction.getOperands();
+ if (filterKind == FilterKind.AND || filterKind == FilterKind.OR) {
+ operands.replaceAll(this::optimize);
+ } else if (filterKind.isRange() || filterKind == FilterKind.EQUALS) {
+ Expression expression = operands.get(0);
+ if (expression.getType() == ExpressionType.FUNCTION) {
+ Function expressionFunction = expression.getFunctionCall();
+ String functionName = StringUtils.remove(expressionFunction.getOperator(), '_');
+ if (functionName.equalsIgnoreCase(TimeConversionTransformFunction.FUNCTION_NAME)) {
+ optimizeTimeConvert(filterFunction, filterKind);
+ } else if (functionName.equalsIgnoreCase(DateTimeConversionTransformFunction.FUNCTION_NAME)) {
+ optimizeDateTimeConvert(filterFunction, filterKind);
+ }
+ }
+ }
+ return filterExpression;
+ }
+
+ /**
+ * Helper method to optimize TIME_CONVERT function with range/equality predicate to directly apply the predicate to
+ * the inner expression. Changes are applied in-place of the filter function.
+ */
+ private void optimizeTimeConvert(Function filterFunction, FilterKind filterKind) {
+ List<Expression> filterOperands = filterFunction.getOperands();
+ List<Expression> timeConvertOperands = filterOperands.get(0).getFunctionCall().getOperands();
+ Preconditions.checkArgument(timeConvertOperands.size() == 3,
+ "Exactly 3 arguments are required for TIME_CONVERT transform function");
+ Preconditions
+ .checkArgument(isStringLiteral(timeConvertOperands.get(1)) && isStringLiteral(timeConvertOperands.get(2)),
+ "The 2nd and 3rd argument for TIME_CONVERT transform function must be string literal");
+
+ try {
+ TimeUnit inputTimeUnit = TimeUnit.valueOf(timeConvertOperands.get(1).getLiteral().getStringValue().toUpperCase());
+ TimeUnit outputTimeUnit =
+ TimeUnit.valueOf(timeConvertOperands.get(2).getLiteral().getStringValue().toUpperCase());
+
+ // Step 1: Convert output range to millis range
+ Long lowerMillis = null;
+ Long upperMillis = null;
+ switch (filterKind) {
+ case GREATER_THAN: {
+ // millisToFormat(millis) > n
+ // -> millisToFormat(millis) >= n + 1
+ // -> millis >= formatToMillis(n + 1)
Review comment:
Not sure why you need to change the input value here by doing +1. Seems like all that needs to be done here is to properly set lowerInclusive boolean value?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6957: Optimize TIME_CONVERT/DATE_TIME_CONVERT predicates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6957:
URL: https://github.com/apache/incubator-pinot/pull/6957#discussion_r637276388
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/filter/TimePredicateFilterOptimizerTest.java
##########
@@ -0,0 +1,163 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.optimizer.filter;
+
+import java.util.List;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.core.query.optimizer.filter.utils.Range;
+import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class TimePredicateFilterOptimizerTest {
+ private static final TimePredicateFilterOptimizer OPTIMIZER = new TimePredicateFilterOptimizer();
+
+ @Test
+ public void testTimeConvert() {
+ testTimeConvert("timeConvert(col, 'MILLISECONDS', 'MILLISECONDS') > 1620830760000",
+ new Range(1620830760001L, true, null, false));
Review comment:
The algorithm is to handle the general case, so as long as the optimized one is correct we should be good.
For the super large/small value, I'll add a check that the millis is in valid range. If not, the optimizer won't change the filter.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org