Jackie-Jiang commented on a change in pull request #8451:
URL: https://github.com/apache/pinot/pull/8451#discussion_r840124465
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/request/context/predicate/RangePredicate.java
##########
@@ -138,4 +138,12 @@ public String toString() {
return "(" + _lhs + (_lowerInclusive ? " >= '" : " > '") + _lowerBound +
"' AND " + _lhs + (_upperInclusive
? " <= '" : " < '") + _upperBound + "')";
}
+
+ /**
+ * @return Generated range string, this string has the same format used in
the Constructor.
+ */
+ public String getRange() {
Review comment:
No need to have this extra ser/de if lhs can be set
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java
##########
@@ -411,4 +415,69 @@ public static FilterContext getFilter(FilterQueryTree
node) {
throw new IllegalStateException();
}
}
+
+ public static FilterContext
overrideFilterWithExpressionOverrideHints(FilterContext filter, IndexSegment
indexSegment,
+ Map<ExpressionContext, ExpressionContext> getExpressionOverrideHints) {
+ if (filter.getChildren() != null) {
+ // AND, OR, NOT
+ List<FilterContext> newChildren = filter.getChildren().stream().map(
+ filterContext ->
overrideFilterWithExpressionOverrideHints(filterContext, indexSegment,
+ getExpressionOverrideHints))
+ .collect(Collectors.toList());
+ return new FilterContext(filter.getType(), newChildren, null);
+ }
+ // PREDICATE
+ Predicate predicate = filter.getPredicate();
+ ExpressionContext newPredicateLhs =
+ overrideWithExpressionHints(indexSegment, getExpressionOverrideHints,
predicate.getLhs());
+ Predicate newPredicate = getNewPredicate(predicate, newPredicateLhs);
+ return new FilterContext(filter.getType(), null, newPredicate);
+ }
+
+ public static Predicate getNewPredicate(Predicate predicate,
ExpressionContext lhs) {
Review comment:
To save garbage, we can add `setLhs()` to all predicates. Consider
adding a `BasePredicate` with `setLhs()` and `getLhs()` and we don't need the
switch
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java
##########
@@ -411,4 +415,69 @@ public static FilterContext getFilter(FilterQueryTree
node) {
throw new IllegalStateException();
}
}
+
+ public static FilterContext
overrideFilterWithExpressionOverrideHints(FilterContext filter, IndexSegment
indexSegment,
+ Map<ExpressionContext, ExpressionContext> getExpressionOverrideHints) {
Review comment:
```suggestion
public static FilterContext overrideWithExpressionHints(FilterContext
filter, IndexSegment indexSegment,
Map<ExpressionContext, ExpressionContext> expressionOverrideHints) {
```
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java
##########
@@ -411,4 +415,69 @@ public static FilterContext getFilter(FilterQueryTree
node) {
throw new IllegalStateException();
}
}
+
+ public static FilterContext
overrideFilterWithExpressionOverrideHints(FilterContext filter, IndexSegment
indexSegment,
+ Map<ExpressionContext, ExpressionContext> getExpressionOverrideHints) {
+ if (filter.getChildren() != null) {
+ // AND, OR, NOT
+ List<FilterContext> newChildren = filter.getChildren().stream().map(
+ filterContext ->
overrideFilterWithExpressionOverrideHints(filterContext, indexSegment,
+ getExpressionOverrideHints))
+ .collect(Collectors.toList());
+ return new FilterContext(filter.getType(), newChildren, null);
+ }
+ // PREDICATE
+ Predicate predicate = filter.getPredicate();
+ ExpressionContext newPredicateLhs =
+ overrideWithExpressionHints(indexSegment, getExpressionOverrideHints,
predicate.getLhs());
+ Predicate newPredicate = getNewPredicate(predicate, newPredicateLhs);
+ return new FilterContext(filter.getType(), null, newPredicate);
+ }
+
+ public static Predicate getNewPredicate(Predicate predicate,
ExpressionContext lhs) {
+ switch (predicate.getType()) {
+ case EQ:
+ return new EqPredicate(lhs, ((EqPredicate) predicate).getValue());
+ case NOT_EQ:
+ return new NotEqPredicate(lhs, ((NotEqPredicate)
predicate).getValue());
+ case IN:
+ return new InPredicate(lhs, ((InPredicate) predicate).getValues());
+ case NOT_IN:
+ return new NotInPredicate(lhs, ((NotInPredicate)
predicate).getValues());
+ case RANGE:
+ return new RangePredicate(lhs, ((RangePredicate)
predicate).getRange());
+ case REGEXP_LIKE:
+ return new RegexpLikePredicate(lhs, ((RegexpLikePredicate)
predicate).getValue());
+ case TEXT_MATCH:
+ return new TextMatchPredicate(lhs, ((TextMatchPredicate)
predicate).getValue());
+ case JSON_MATCH:
+ return new JsonMatchPredicate(lhs, ((JsonMatchPredicate)
predicate).getValue());
+ case IS_NULL:
+ return new IsNullPredicate(lhs);
+ case IS_NOT_NULL:
+ return new IsNotNullPredicate(lhs);
+ default:
+ throw new IllegalStateException();
+ }
+ }
+
+ public static ExpressionContext overrideWithExpressionHints(IndexSegment
indexSegment,
+ Map<ExpressionContext, ExpressionContext> expressionOverrideHints,
ExpressionContext expression) {
Review comment:
(minor) Put `expression` as the first argument for consistency
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java
##########
@@ -411,4 +415,69 @@ public static FilterContext getFilter(FilterQueryTree
node) {
throw new IllegalStateException();
}
}
+
+ public static FilterContext
overrideFilterWithExpressionOverrideHints(FilterContext filter, IndexSegment
indexSegment,
+ Map<ExpressionContext, ExpressionContext> getExpressionOverrideHints) {
+ if (filter.getChildren() != null) {
+ // AND, OR, NOT
+ List<FilterContext> newChildren = filter.getChildren().stream().map(
+ filterContext ->
overrideFilterWithExpressionOverrideHints(filterContext, indexSegment,
+ getExpressionOverrideHints))
+ .collect(Collectors.toList());
+ return new FilterContext(filter.getType(), newChildren, null);
+ }
+ // PREDICATE
+ Predicate predicate = filter.getPredicate();
+ ExpressionContext newPredicateLhs =
+ overrideWithExpressionHints(indexSegment, getExpressionOverrideHints,
predicate.getLhs());
+ Predicate newPredicate = getNewPredicate(predicate, newPredicateLhs);
+ return new FilterContext(filter.getType(), null, newPredicate);
+ }
+
+ public static Predicate getNewPredicate(Predicate predicate,
ExpressionContext lhs) {
+ switch (predicate.getType()) {
+ case EQ:
+ return new EqPredicate(lhs, ((EqPredicate) predicate).getValue());
+ case NOT_EQ:
+ return new NotEqPredicate(lhs, ((NotEqPredicate)
predicate).getValue());
+ case IN:
+ return new InPredicate(lhs, ((InPredicate) predicate).getValues());
+ case NOT_IN:
+ return new NotInPredicate(lhs, ((NotInPredicate)
predicate).getValues());
+ case RANGE:
+ return new RangePredicate(lhs, ((RangePredicate)
predicate).getRange());
+ case REGEXP_LIKE:
+ return new RegexpLikePredicate(lhs, ((RegexpLikePredicate)
predicate).getValue());
+ case TEXT_MATCH:
+ return new TextMatchPredicate(lhs, ((TextMatchPredicate)
predicate).getValue());
+ case JSON_MATCH:
+ return new JsonMatchPredicate(lhs, ((JsonMatchPredicate)
predicate).getValue());
+ case IS_NULL:
+ return new IsNullPredicate(lhs);
+ case IS_NOT_NULL:
+ return new IsNotNullPredicate(lhs);
+ default:
+ throw new IllegalStateException();
+ }
+ }
+
+ public static ExpressionContext overrideWithExpressionHints(IndexSegment
indexSegment,
+ Map<ExpressionContext, ExpressionContext> expressionOverrideHints,
ExpressionContext expression) {
+ if (expression.getType() != ExpressionContext.Type.FUNCTION) {
+ return expression;
+ }
+ ExpressionContext overrideExpression =
expressionOverrideHints.get(expression);
+ if (overrideExpression != null && indexSegment.getPhysicalColumnNames()
Review comment:
Check if `overrideExpression` is identifier before looking up the map
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java
##########
@@ -411,4 +415,69 @@ public static FilterContext getFilter(FilterQueryTree
node) {
throw new IllegalStateException();
}
}
+
+ public static FilterContext
overrideFilterWithExpressionOverrideHints(FilterContext filter, IndexSegment
indexSegment,
+ Map<ExpressionContext, ExpressionContext> getExpressionOverrideHints) {
+ if (filter.getChildren() != null) {
+ // AND, OR, NOT
+ List<FilterContext> newChildren = filter.getChildren().stream().map(
+ filterContext ->
overrideFilterWithExpressionOverrideHints(filterContext, indexSegment,
+ getExpressionOverrideHints))
+ .collect(Collectors.toList());
+ return new FilterContext(filter.getType(), newChildren, null);
+ }
+ // PREDICATE
+ Predicate predicate = filter.getPredicate();
+ ExpressionContext newPredicateLhs =
+ overrideWithExpressionHints(indexSegment, getExpressionOverrideHints,
predicate.getLhs());
+ Predicate newPredicate = getNewPredicate(predicate, newPredicateLhs);
+ return new FilterContext(filter.getType(), null, newPredicate);
+ }
+
+ public static Predicate getNewPredicate(Predicate predicate,
ExpressionContext lhs) {
+ switch (predicate.getType()) {
+ case EQ:
+ return new EqPredicate(lhs, ((EqPredicate) predicate).getValue());
+ case NOT_EQ:
+ return new NotEqPredicate(lhs, ((NotEqPredicate)
predicate).getValue());
+ case IN:
+ return new InPredicate(lhs, ((InPredicate) predicate).getValues());
+ case NOT_IN:
+ return new NotInPredicate(lhs, ((NotInPredicate)
predicate).getValues());
+ case RANGE:
+ return new RangePredicate(lhs, ((RangePredicate)
predicate).getRange());
+ case REGEXP_LIKE:
+ return new RegexpLikePredicate(lhs, ((RegexpLikePredicate)
predicate).getValue());
+ case TEXT_MATCH:
+ return new TextMatchPredicate(lhs, ((TextMatchPredicate)
predicate).getValue());
+ case JSON_MATCH:
+ return new JsonMatchPredicate(lhs, ((JsonMatchPredicate)
predicate).getValue());
+ case IS_NULL:
+ return new IsNullPredicate(lhs);
+ case IS_NOT_NULL:
+ return new IsNotNullPredicate(lhs);
+ default:
+ throw new IllegalStateException();
+ }
+ }
+
+ public static ExpressionContext overrideWithExpressionHints(IndexSegment
indexSegment,
+ Map<ExpressionContext, ExpressionContext> expressionOverrideHints,
ExpressionContext expression) {
+ if (expression.getType() != ExpressionContext.Type.FUNCTION) {
+ return expression;
+ }
+ ExpressionContext overrideExpression =
expressionOverrideHints.get(expression);
+ if (overrideExpression != null && indexSegment.getPhysicalColumnNames()
+ .contains(overrideExpression.getIdentifier())) {
+ return overrideExpression;
+ }
+ List<ExpressionContext> newArguments = new ArrayList<>();
Review comment:
We can do in-place replacement to save garbage
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java
##########
@@ -411,4 +415,69 @@ public static FilterContext getFilter(FilterQueryTree
node) {
throw new IllegalStateException();
}
}
+
+ public static FilterContext
overrideFilterWithExpressionOverrideHints(FilterContext filter, IndexSegment
indexSegment,
Review comment:
We should move these data dependent methods to a separate util (or put
them in `InstancePlanMakerImplV2` for now, and extract them out in the future
if needed to be reused). IMO `RequestContextUtils` should be data independent,
and only related to query.
##########
File path:
pinot-common/src/test/java/org/apache/pinot/common/request/RequestContextUtilsTest.java
##########
@@ -0,0 +1,191 @@
+/**
+ * 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.common.request;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.request.context.FilterContext;
+import org.apache.pinot.common.request.context.FunctionContext;
+import org.apache.pinot.common.request.context.RequestContextUtils;
+import org.apache.pinot.common.request.context.predicate.EqPredicate;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.SegmentMetadata;
+import org.apache.pinot.segment.spi.datasource.DataSource;
+import
org.apache.pinot.segment.spi.index.mutable.ThreadSafeMutableRoaringBitmap;
+import org.apache.pinot.segment.spi.index.startree.StarTreeV2;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class RequestContextUtilsTest {
+ private IndexSegment _indexSegment = new IndexSegment() {
+ @Override
+ public String getSegmentName() {
+ return null;
+ }
+
+ @Override
+ public SegmentMetadata getSegmentMetadata() {
+ return null;
+ }
+
+ @Override
+ public Set<String> getColumnNames() {
+ return null;
+ }
+
+ @Override
+ public Set<String> getPhysicalColumnNames() {
+ return ImmutableSet.of("$ts$MONTH");
+ }
+
+ @Override
+ public DataSource getDataSource(String columnName) {
+ return null;
+ }
+
+ @Override
+ public List<StarTreeV2> getStarTrees() {
+ return null;
+ }
+
+ @Nullable
+ @Override
+ public ThreadSafeMutableRoaringBitmap getValidDocIds() {
+ return null;
+ }
+
+ @Override
+ public GenericRow getRecord(int docId, GenericRow reuse) {
+ return null;
+ }
+
+ @Override
+ public void destroy() {
+
+ }
+ };
+
+ @Test
+ public void testExpressionContextHashcode() {
+ ExpressionContext expressionContext1 =
ExpressionContext.forIdentifier("abc");
+ ExpressionContext expressionContext2 =
ExpressionContext.forIdentifier("abc");
+ Assert.assertEquals(expressionContext1, expressionContext2);
Review comment:
(minor) We may static import assert functions in tests
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -248,6 +255,48 @@ public PlanNode makeSegmentPlanNode(IndexSegment
indexSegment, QueryContext quer
}
}
+ private QueryContext rewriteExpressionsWithHints(IndexSegment indexSegment,
QueryContext queryContext) {
+ List<ExpressionContext> newSelectExpressions =
queryContext.getSelectExpressions();
+ if (newSelectExpressions != null && !newSelectExpressions.isEmpty()) {
+ newSelectExpressions = newSelectExpressions.stream().map(
Review comment:
We can do in-place replacement to save garbage. Also suggest not using
stream api for performance
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
