yashmayya commented on code in PR #14690:
URL: https://github.com/apache/pinot/pull/14690#discussion_r1897674620
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java:
##########
@@ -344,6 +347,17 @@ public static void
rewriteQueryContextWithHints(QueryContext queryContext, Index
selectExpressions.replaceAll(
expression -> overrideWithExpressionHints(expression, indexSegment,
expressionOverrideHints));
+ List<Pair<AggregationFunction, FilterContext>> filtAggrFuns =
queryContext.getFilteredAggregationFunctions();
+ if (filtAggrFuns != null) {
+ for (Pair<AggregationFunction, FilterContext>
filteredAggregationFunction : filtAggrFuns) {
+ FilterContext right = filteredAggregationFunction.getRight();
+ if (right != null) {
+ Predicate predicate = right.getPredicate();
+ predicate.setLhs(overrideWithExpressionHints(predicate.getLhs(),
indexSegment, expressionOverrideHints));
+ }
+ }
+ }
Review Comment:
Could we add an integration test that verifies this filtered aggregation on
timestamp column with timestamp index execution path on both the query engines?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestVisitor.java:
##########
@@ -240,4 +250,14 @@ private boolean visit(PlanNode node,
ServerPlanRequestContext context) {
node.visit(this, context);
return context.getLeafStageBoundaryNode() == null;
}
+
+ private void applyTimestampIndex(Expression expression, PinotQuery
pinotQuery) {
+ RequestUtils.applyTimestampIndex(expression, pinotQuery);
+ Function functionCall = expression.getFunctionCall();
+ if (expression.isSetFunctionCall()) {
+ for (Expression operand : functionCall.getOperands()) {
+ applyTimestampIndex(operand, pinotQuery);
+ }
+ }
+ }
Review Comment:
So here we're adding the expression override hints regardless of the actual
timestamp index configurations and relying on the server to only apply the
correct ones?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -935,24 +934,7 @@ private void
setTimestampIndexExpressionOverrideHints(@Nullable Expression expre
return;
}
Function function = expression.getFunctionCall();
- switch (function.getOperator()) {
- case "datetrunc":
- String granularString =
function.getOperands().get(0).getLiteral().getStringValue().toUpperCase();
- Expression timeExpression = function.getOperands().get(1);
- if (((function.getOperandsSize() == 2) || (function.getOperandsSize()
== 3 && "MILLISECONDS".equalsIgnoreCase(
- function.getOperands().get(2).getLiteral().getStringValue()))) &&
TimestampIndexUtils.isValidGranularity(
- granularString) && timeExpression.getIdentifier() != null) {
- String timeColumn = timeExpression.getIdentifier().getName();
- String timeColumnWithGranularity =
TimestampIndexUtils.getColumnWithGranularity(timeColumn, granularString);
- if (timestampIndexColumns.contains(timeColumnWithGranularity)) {
- pinotQuery.putToExpressionOverrideHints(expression,
-
RequestUtils.getIdentifierExpression(timeColumnWithGranularity));
- }
- }
- break;
- default:
- break;
- }
+ RequestUtils.applyTimestampIndex(expression, pinotQuery,
timestampIndexColumns::contains);
Review Comment:
```suggestion
RequestUtils.applyTimestampIndexOverrideHints(expression, pinotQuery,
timestampIndexColumns::contains);
```
nit: the current name makes it sound like this is where the timestamp index
itself is being used rather than just an expression rewrite.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ExplainIntegrationTestTrait.java:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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.integration.tests;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import org.intellij.lang.annotations.Language;
+import org.testng.Assert;
+
+
+public interface ExplainIntegrationTestTrait {
+
+ JsonNode postQuery(@Language("sql") String query)
+ throws Exception;
+
+ default void explainLogical(@Language("sql") String query, String expected) {
Review Comment:
Why not add these into `ClusterTest` / `BaseClusterIntegrationTest` /
`BaseClusterIntegrationTestSet` instead of this separate interface that tests
need to additionally extend?
##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java:
##########
@@ -91,9 +91,10 @@ public abstract class BaseClusterIntegrationTest extends
ClusterTest {
protected static final List<String> DEFAULT_NO_DICTIONARY_COLUMNS =
Arrays.asList("ActualElapsedTime", "ArrDelay", "DepDelay", "CRSDepTime");
protected static final String DEFAULT_SORTED_COLUMN = "Carrier";
- protected static final List<String> DEFAULT_INVERTED_INDEX_COLUMNS =
Arrays.asList("FlightNum", "Origin", "Quarter");
- private static final List<String> DEFAULT_BLOOM_FILTER_COLUMNS =
Arrays.asList("FlightNum", "Origin");
- private static final List<String> DEFAULT_RANGE_INDEX_COLUMNS =
Collections.singletonList("Origin");
+ protected static final List<String> DEFAULT_INVERTED_INDEX_COLUMNS
+ = Lists.newArrayList("FlightNum", "Origin", "Quarter");
+ private static final List<String> DEFAULT_BLOOM_FILTER_COLUMNS =
Lists.newArrayList("FlightNum", "Origin");
+ private static final List<String> DEFAULT_RANGE_INDEX_COLUMNS =
Lists.newArrayList("Origin");
Review Comment:
Is this change to make the lists mutable? Are we mutating them anywhere?
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java:
##########
@@ -631,4 +633,32 @@ public static Map<String, String>
getOptionsFromJson(JsonNode request, String op
public static Map<String, String> getOptionsFromString(String optionStr) {
return
Splitter.on(';').omitEmptyStrings().trimResults().withKeyValueSeparator('=').split(optionStr);
}
+
+ public static void applyTimestampIndex(Expression expression, PinotQuery
query) {
+ applyTimestampIndex(expression, query, timeColumnWithGranularity -> true);
+ }
+
+ public static void applyTimestampIndex(
+ Expression expression, PinotQuery query, Predicate<String>
timeColumnWithGranularityPredicate
+ ) {
+ if (!expression.isSetFunctionCall()) {
+ return;
+ }
+ Function function = expression.getFunctionCall();
+ if (!function.getOperator().equalsIgnoreCase("datetrunc")) {
Review Comment:
Can use `TransformFunctionType.DATE_TRUNC.getName()` here to avoid
hard-coding the constant.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
