You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "MeihanLi (via GitHub)" <gi...@apache.org> on 2025/11/10 18:17:09 UTC
[PR] Add Query Fingerprint Feature [pinot]
MeihanLi opened a new pull request, #17177:
URL: https://github.com/apache/pinot/pull/17177
### Add SqlNode-based Query Fingerprinting for Multi-Stage Query Engine
**Summary**
This PR introduces a robust SqlNode-based query fingerprinting mechanism that works for both single-stage and multi-stage query engines. Query fingerprinting creates canonical representations of SQL queries by replacing literals with dynamic parameters, enabling query pattern analysis, caching, and monitoring.
**Issues to Solve**
https://github.com/apache/pinot/issues/16765
**Details:**
Parses queries using Calcite's SqlNode AST - Works with the same parser used by the multi-stage engine
Normalizes queries via visitor pattern - Traverses the AST and replaces all literals with dynamic parameters (?)
Supports all SQL constructs - Handles table aliases, JOINs, CTEs, subqueries, set operations, window functions, and more
Produces consistent fingerprints - Normalizes whitespace and uses ANSI SQL standard syntax
**Release Notes**
`feature` `observability`
By default, query fingerprinting is disabled. This is a non-breaking change that requires opt-in configuration to enable the feature in production environments.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2525543680
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintVisitor.java:
##########
@@ -0,0 +1,297 @@
+/**
+ * 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.utils.request;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlAsOperator;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlJoin;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlNumericLiteral;
+import org.apache.calcite.sql.SqlOrderBy;
+import org.apache.calcite.sql.SqlOverOperator;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.fun.SqlCase;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+
+/**
+ * Query fingerprint visitor that traverses the Calcite SqlNode AST and replaces all literals with
+ * dynamic parameters (?), producing a normalized query suitable for fingerprinting.
+ */
+public class QueryFingerprintVisitor extends SqlShuttle {
+ private static final long MISSING_LIMIT_VALUE = Long.MIN_VALUE;
+
+ private boolean _isAlias = false;
+ private boolean _isInFrom = false;
+ private long _maxLimit = MISSING_LIMIT_VALUE;
+
+ /** Use tree-set for storing table names so metrics are emitted in consistent order. */
+ private final Set<String> _tableNames = new TreeSet<>();
+ private final Set<String> _aliases = new HashSet<>();
+ private int _dynamicParamIndex = 0;
+
+ @Override
+ public SqlNode visit(SqlLiteral literal) {
+ // Skip keyword/symbol literals (DISTINCT, NULL, TRUE, FALSE, UNKNOWN, etc.)
+ // These should be preserved in the fingerprint
+ if (literal.getTypeName() == SqlTypeName.SYMBOL
+ || literal.getTypeName() == SqlTypeName.NULL
+ || literal.getTypeName() == SqlTypeName.BOOLEAN) {
+ return literal;
+ }
+ // Replace data literals (numbers, strings, dates, etc.) with dynamic parameters
+ return new SqlDynamicParam(_dynamicParamIndex++, literal.getParserPosition());
+ }
+
+ @Override
+ public @Nullable SqlNode visit(SqlCall call) {
+ SqlNode result = call;
+ switch (call.getKind()) {
+ case SELECT:
+ result = visitSelect((SqlSelect) call);
+ break;
+ case JOIN:
+ result = visitJoin((SqlJoin) call);
+ break;
+ case WITH:
+ result = visitWith((SqlWith) call);
+ break;
+ case WITH_ITEM:
+ result = visitWithItem((SqlWithItem) call);
+ break;
+ case EXPLAIN:
+ result = call.getOperandList().get(0).accept(this);
+ break;
+ case CASE:
+ result = visitCase((SqlCase) call);
+ break;
+ case ORDER_BY:
+ result = visitOrderBy((SqlOrderBy) call);
+ break;
+ // Set operations: UNION, INTERSECT, EXCEPT (all supported by Pinot multi-stage engine)
+ case UNION:
+ case INTERSECT:
+ case EXCEPT:
+ case MINUS:
+ // These are binary operators with two query operands - let default super.visit() handle them
+ return super.visit(call);
+ default:
+ if (call.getOperator() instanceof SqlAsOperator) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ SqlNode firstOperand = call.getOperandList().get(0);
+ SqlNode secondOperand = call.getOperandList().get(1);
+ boolean isAliasBackup = _isAlias;
+ if (firstOperand != null) {
+ newOperands.add(firstOperand.accept(this));
+ } else {
+ newOperands.add(null);
+ }
+ _isAlias = true;
+ if (secondOperand != null) {
+ newOperands.add(secondOperand.accept(this));
+ } else {
+ newOperands.add(null);
+ }
+ _isAlias = isAliasBackup;
+ call.setOperand(0, newOperands.get(0));
+ call.setOperand(1, newOperands.get(1));
+ } else if (call.getOperator() instanceof SqlOverOperator) {
+ // Do not visit the window function
+ // SqlOver operator has 2 operands, the first one is the aggregate function and
+ // the second one is the window.
+ // Window function is whatever is inside OVER() clause. It can be empty () too.
+ // For now, Pinot supports partition by, order by and limited support for window frame.
+ // We can revisit traversing window function if we start finding literals
+ // inside frame clause of window function. Partition by, Order by will not have literal
+ // clauses.
+ call.setOperand(0, call.getOperandList().get(0).accept(this));
+ } else {
+ return super.visit(call);
+ }
+ break;
+ }
+ return result;
+ }
+
+ @Nullable
+ private SqlNode visitCase(SqlCase sqlCase) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : sqlCase.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ newOperands.add(child.accept(this));
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ sqlCase.setOperand(i++, operand);
+ }
+ return sqlCase;
+ }
+
+ @Nullable
+ private SqlNode visitSelect(SqlSelect select) {
+ boolean isInFromBackup = _isInFrom;
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : select.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ _isInFrom = select.getFrom() == child;
+ newOperands.add(child.accept(this));
+ }
+ _isInFrom = isInFromBackup;
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // The 11th node is a `hints` variable of type SqlNodeList, which is more complex to traverse
+ // (as seen in how `columnList` vs `query` is handled in SqlWith). Since SQL hints are
+ // discouraged in Pinot, this was intentionally skipped.
+ if (i == 11) {
Review Comment:
Added public static constants with java docs.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2618392546
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintVisitor.java:
##########
@@ -0,0 +1,307 @@
+/**
+ * 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.utils.request;
+
+import java.util.ArrayList;
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlJoin;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlOrderBy;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.fun.SqlCase;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+
+/**
+ * QueryFingerprintVisitor traverses the Calcite SqlNode AST and produces a normalized query fingerprint.
+ * Implementation is based on Calcite 1.40.0 version. It may change in future versions of Calcite.
+ *
+ * <p>
+ * <ul>
+ * <li>All data literals are replaced with dynamic parameters (?)</li>
+ * <li>IN/NOT IN clauses with multiple constants are squashed to a single parameter.</li>
+ * <li>EXPLAIN PLAN FOR is NOT preserved.</li>
+ * <li>Symbolic keywords (DISTINCT, ASC, DESC, etc.) and NULL literals are preserved.</li>
+ * <li>Hints are preserved.</li>
+ * <li>Window functions: window specification (operand 1) are preserved.</li>
+ * </ul>
+ * </p>
+ */
+public class QueryFingerprintVisitor extends SqlShuttle {
+ // SqlSelect operand index for hints, see {@link org.apache.calcite.sql.SqlSelect}.
+ private static final int SQLSELECT_HINTS_OPERAND_INDEX = 11;
+
+ // SqlJoin operand indices, see {@link org.apache.calcite.sql.SqlJoin}.
+ private static final int SQLJOIN_NATURAL_OPERAND_INDEX = 1;
+ private static final int SQLJOIN_JOIN_TYPE_OPERAND_INDEX = 2;
+ private static final int SQLJOIN_CONDITION_TYPE_OPERAND_INDEX = 4;
+
+ private int _dynamicParamIndex = 0;
+
+ @Override
Review Comment:
The `_dynamicParamIndex` field is never reset between query fingerprints. If the same visitor instance is reused for multiple queries, the dynamic parameter indices will continue incrementing. Consider either documenting that each visitor instance should only be used once, or resetting `_dynamicParamIndex` at the start of the visit process.
```suggestion
@Override
public SqlNode visit(SqlNode node) {
_dynamicParamIndex = 0;
return super.visit(node);
}
@Override
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -311,11 +331,11 @@ protected BrokerResponse handleRequestThrowing(long requestId, String query, Sql
QueryExecutionContext executionContext =
new QueryExecutionContext(QueryExecutionContext.QueryType.MSE, requestId, cid, workloadName, startTimeMs,
- activeDeadlineMs, passiveDeadlineMs, _brokerId, _brokerId);
+ activeDeadlineMs, passiveDeadlineMs, _brokerId, _brokerId, queryHash);
QueryThreadContext.MseWorkerInfo mseWorkerInfo = new QueryThreadContext.MseWorkerInfo(0, 0);
try (QueryThreadContext ignore = QueryThreadContext.open(executionContext, mseWorkerInfo, _threadAccountant);
Review Comment:
The queryHash registered in MDC at line 313 is never unregistered after the request completes. This creates a resource leak where MDC entries accumulate. Since QueryThreadContext is responsible for MDC cleanup (as seen in its close() method), and executionContext now contains the queryHash, the MDC registration should occur inside the try-with-resources block where QueryThreadContext.open is called, not before it.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -340,7 +359,8 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
// TODO: Revisit whether we should set deadline here
QueryExecutionContext executionContext =
new QueryExecutionContext(QueryExecutionContext.QueryType.SSE, requestId, cid, workloadName,
- requestContext.getRequestArrivalTimeMillis(), Long.MAX_VALUE, Long.MAX_VALUE, _brokerId, _brokerId);
+ requestContext.getRequestArrivalTimeMillis(), Long.MAX_VALUE, Long.MAX_VALUE, _brokerId, _brokerId,
+ queryHash);
try (QueryThreadContext ignore = QueryThreadContext.open(executionContext, _threadAccountant)) {
Review Comment:
The queryHash registered in MDC at line 344 is never unregistered after the request completes. This creates a resource leak where MDC entries accumulate. Since QueryThreadContext is responsible for MDC cleanup (as seen in its close() method), and executionContext now contains the queryHash, the MDC registration should occur inside the try-with-resources block where QueryThreadContext.open is called, not before 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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2625616061
##########
pinot-spi/src/main/java/org/apache/pinot/spi/query/QueryThreadContext.java:
##########
@@ -129,6 +133,9 @@ public void close() {
THREAD_LOCAL.remove();
LoggerConstants.REQUEST_ID_KEY.unregisterFromMdc();
LoggerConstants.CORRELATION_ID_KEY.unregisterFromMdc();
+ if (_executionContext.getQueryHash() != null) {
+ LoggerConstants.QUERY_HASH_KEY.unregisterFromMdc();
+ }
Review Comment:
Fixed in the most recent commit.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2535274860
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintVisitor.java:
##########
@@ -0,0 +1,214 @@
+/**
+ * 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.utils.request;
+
+import java.util.ArrayList;
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlJoin;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlOrderBy;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.fun.SqlCase;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+
+/**
+ * QueryFingerprintVisitor traverses the Calcite SqlNode AST and produces a normalized query fingerprint.
+ * Implementation is based on Calcite 1.40.0 version. It may change in future versions of Calcite.
+ *
+ * <p>
+ * <ul>
+ * <li>All data literals are replaced with dynamic parameters (?)</li>
+ * <li>Symbolic keywords (DISTINCT, ASC, DESC, etc.) and NULL literals are preserved.</li>
+ * <li>Hints are preserved.</li>
+ * <li>EXPLAIN PLAN FOR is NOT preserved.</li>
+ * </ul>
+ * </p>
+ */
+public class QueryFingerprintVisitor extends SqlShuttle {
+ // SqlSelect operand index for hints, see {@link org.apache.calcite.sql.SqlSelect}.
+ private static final int HINTS_OPERAND_INDEX = 11;
Review Comment:
Let's prefix it with the operand name as we can have the same field for multiple operands.
i.e.
```
private static final SQLSELECT_HINTS_OEPRAND_INDEX = 11;
```
and the same for below.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on PR #17177:
URL: https://github.com/apache/pinot/pull/17177#issuecomment-3544143207
Thanks @kishoreg for the comment.
Some examples for IN clause below. You can also check QueryFingerprintUtilsTest.java and QueryFingerprintVisitorTest.java for more query examples including both Single Stage query and MSE queries.
```
"SELECT col1 FROM table1 WHERE col2 IN (100, 200, 300)"
=> "SELECT `col1` FROM `table1` WHERE `col2` IN (?, ?, ?)"
"SELECT col1 FROM table1 WHERE col2 IN (SELECT col2 FROM table2 WHERE col3 = 100)"
=> "SELECT `col1` FROM `table1` WHERE `col2` IN (SELECT `col2` FROM `table2` WHERE `col3` = ?)"
```
Regarding plan caching by fingerprint:
Literals are currently baked directly into the plan (e.g., RexExpression.Literal, FilterNode, DispatchableSubPlan), so we can’t substitute them at execution time. To reuse a plan with different literals, we’d need to clone the entire tree and rewrite all literal-bearing nodes. We also can’t safely cache post-optimization plans because many rules are literal-dependent (constant folding, dead-code elimination, etc.), and reusing an optimized plan with different literal values can yield incorrect results.
Caching at the pre-optimization RelRoot level is possible, but it only skips validation + conversion. The expensive optimization phase would still run for each query.
Given that, fingerprints are most useful today for metrics, logging, and identifying common patterns. If we see strong demand, we could start by caching plans for identical queries before taking on the bigger architectural change required for true parameterized plan caching.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "cbalci (via GitHub)" <gi...@apache.org>.
cbalci commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2543672064
##########
pinot-spi/src/main/java/org/apache/pinot/spi/trace/QueryFingerprint.java:
##########
@@ -0,0 +1,37 @@
+/**
+ * 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.spi.trace;
+
+public class QueryFingerprint {
Review Comment:
Would be useful to implement `equals` and `hashCode` methods for this class. It could make comparisons easier.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2625629818
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintVisitor.java:
##########
@@ -0,0 +1,307 @@
+/**
+ * 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.utils.request;
+
+import java.util.ArrayList;
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlJoin;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlOrderBy;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.fun.SqlCase;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+
+/**
+ * QueryFingerprintVisitor traverses the Calcite SqlNode AST and produces a normalized query fingerprint.
+ * Implementation is based on Calcite 1.40.0 version. It may change in future versions of Calcite.
+ *
+ * <p>
+ * <ul>
+ * <li>All data literals are replaced with dynamic parameters (?)</li>
+ * <li>IN/NOT IN clauses with multiple constants are squashed to a single parameter.</li>
+ * <li>EXPLAIN PLAN FOR is NOT preserved.</li>
+ * <li>Symbolic keywords (DISTINCT, ASC, DESC, etc.) and NULL literals are preserved.</li>
+ * <li>Hints are preserved.</li>
+ * <li>Window functions: window specification (operand 1) are preserved.</li>
+ * </ul>
+ * </p>
+ */
+public class QueryFingerprintVisitor extends SqlShuttle {
+ // SqlSelect operand index for hints, see {@link org.apache.calcite.sql.SqlSelect}.
+ private static final int SQLSELECT_HINTS_OPERAND_INDEX = 11;
+
+ // SqlJoin operand indices, see {@link org.apache.calcite.sql.SqlJoin}.
+ private static final int SQLJOIN_NATURAL_OPERAND_INDEX = 1;
+ private static final int SQLJOIN_JOIN_TYPE_OPERAND_INDEX = 2;
+ private static final int SQLJOIN_CONDITION_TYPE_OPERAND_INDEX = 4;
+
+ private int _dynamicParamIndex = 0;
+
+ @Override
Review Comment:
Good catch! The current usage in QueryFingerprintUtils already creates a new instance for each fingerprint generation, so it's safe. I've added documentation to the class to clarify that a new instance should be created for each query fingerprint.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2551157983
##########
pinot-spi/src/main/java/org/apache/pinot/spi/trace/QueryFingerprint.java:
##########
@@ -0,0 +1,37 @@
+/**
+ * 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.spi.trace;
+
+public class QueryFingerprint {
Review Comment:
Thanks Caner, I added equals, hashCode and toString with UTs
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on PR #17177:
URL: https://github.com/apache/pinot/pull/17177#issuecomment-3564913513
> > ```
> > "SELECT col1 FROM table1 WHERE col2 IN (100, 200, 300)"
> > => "SELECT `col1` FROM `table1` WHERE `col2` IN (?, ?, ?)"
> > "SELECT col1 FROM table1 WHERE col2 IN (SELECT col2 FROM table2 WHERE col3 = 100)"
> > => "SELECT `col1` FROM `table1` WHERE `col2` IN (SELECT `col2` FROM `table2` WHERE `col3` = ?)"
> > ```
>
> Can we generate only one ? for IN
@kishoreg Thanks for the suggestion, we now generate only one ? for IN/NOT clause with all non-preserved data literals.
Case 1: All values are data literals → replace entire list with a single ?
```
"SELECT col1 FROM table1 WHERE col2 IN (1)";
"SELECT col1 FROM table1 WHERE col2 IN (1, 2, 3)";
"SELECT col1 FROM table1 WHERE col2 IN (5, 10, 15, 20, 25, 30)";
-> SELECT col1 FROM table1 WHERE col2 IN (?)
```
Case 2: Contains any of the following → visit each value individually and replace only data literals
- Preserved literals (NULL, etc.): IN (1, NULL, 3) → IN (?, NULL, ?)
- Expressions: IN (col1 + 1, 2) → IN (col1 + ?, ?)
- Function calls: IN (UPPER('a'), LOWER('b')) → IN (UPPER(?), LOWER(?))
- Subquery: IN (SELECT ...) → visits the subquery normally
You can check QueryFingerprintVisitor::visitIn function and QueryFingerprintVisitorTest for more details.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on PR #17177:
URL: https://github.com/apache/pinot/pull/17177#issuecomment-3565050664
@MeihanLi I am out but wanted to say I am very happy to see this getting delivered in open-source finally 😄
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2625637201
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -311,11 +331,11 @@ protected BrokerResponse handleRequestThrowing(long requestId, String query, Sql
QueryExecutionContext executionContext =
new QueryExecutionContext(QueryExecutionContext.QueryType.MSE, requestId, cid, workloadName, startTimeMs,
- activeDeadlineMs, passiveDeadlineMs, _brokerId, _brokerId);
+ activeDeadlineMs, passiveDeadlineMs, _brokerId, _brokerId, queryHash);
QueryThreadContext.MseWorkerInfo mseWorkerInfo = new QueryThreadContext.MseWorkerInfo(0, 0);
try (QueryThreadContext ignore = QueryThreadContext.open(executionContext, mseWorkerInfo, _threadAccountant);
Review Comment:
Removed the duplicate MDC registration line 313. QueryThreadContext already handles the MDC registration in its constructor and cleanup in its close() method.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -340,7 +359,8 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
// TODO: Revisit whether we should set deadline here
QueryExecutionContext executionContext =
new QueryExecutionContext(QueryExecutionContext.QueryType.SSE, requestId, cid, workloadName,
- requestContext.getRequestArrivalTimeMillis(), Long.MAX_VALUE, Long.MAX_VALUE, _brokerId, _brokerId);
+ requestContext.getRequestArrivalTimeMillis(), Long.MAX_VALUE, Long.MAX_VALUE, _brokerId, _brokerId,
+ queryHash);
try (QueryThreadContext ignore = QueryThreadContext.open(executionContext, _threadAccountant)) {
Review Comment:
same here. Removed the duplicate MDC registration line 344.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #17177:
URL: https://github.com/apache/pinot/pull/17177
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2624684162
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -167,6 +167,10 @@ public static boolean isCollectGcStats(Map<String, String> queryOptions) {
return Boolean.parseBoolean(queryOptions.get(QueryOptionKey.COLLECT_GC_STATS));
}
+ public static String getQueryHash(Map<String, String> queryOptions) {
+ return queryOptions.getOrDefault(QueryOptionKey.QUERY_HASH, CommonConstants.Broker.DEFAULT_QUERY_HASH);
+ }
Review Comment:
There is already a broker-level option to enable it by default. Check CommonConstants.java, we can enable it on broker by setting `pinot.broker.enable.query.fingerprinting = 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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2625645303
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintVisitor.java:
##########
@@ -0,0 +1,307 @@
+/**
+ * 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.utils.request;
+
+import java.util.ArrayList;
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlJoin;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlOrderBy;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.fun.SqlCase;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+
+/**
+ * QueryFingerprintVisitor traverses the Calcite SqlNode AST and produces a normalized query fingerprint.
+ * Implementation is based on Calcite 1.40.0 version. It may change in future versions of Calcite.
+ *
+ * <p>
+ * <ul>
+ * <li>All data literals are replaced with dynamic parameters (?)</li>
+ * <li>IN/NOT IN clauses with multiple constants are squashed to a single parameter.</li>
+ * <li>EXPLAIN PLAN FOR is NOT preserved.</li>
+ * <li>Symbolic keywords (DISTINCT, ASC, DESC, etc.) and NULL literals are preserved.</li>
+ * <li>Hints are preserved.</li>
+ * <li>Window functions: window specification (operand 1) are preserved.</li>
+ * </ul>
+ * </p>
+ */
+public class QueryFingerprintVisitor extends SqlShuttle {
+ // SqlSelect operand index for hints, see {@link org.apache.calcite.sql.SqlSelect}.
+ private static final int SQLSELECT_HINTS_OPERAND_INDEX = 11;
+
+ // SqlJoin operand indices, see {@link org.apache.calcite.sql.SqlJoin}.
+ private static final int SQLJOIN_NATURAL_OPERAND_INDEX = 1;
+ private static final int SQLJOIN_JOIN_TYPE_OPERAND_INDEX = 2;
+ private static final int SQLJOIN_CONDITION_TYPE_OPERAND_INDEX = 4;
+
+ private int _dynamicParamIndex = 0;
+
+ @Override
+ public SqlNode visit(SqlLiteral literal) {
+ if (shouldPreserveLiteral(literal)) {
+ return literal;
+ }
+ // Replace data literals (numbers, strings, dates, etc.) with dynamic parameters
+ return new SqlDynamicParam(_dynamicParamIndex++, literal.getParserPosition());
+ }
+
+ @Override
+ public @Nullable SqlNode visit(SqlCall call) {
+ SqlNode result = call;
+ switch (call.getKind()) {
+ case SELECT:
+ result = visitSelect((SqlSelect) call);
+ break;
+ case JOIN:
+ result = visitJoin((SqlJoin) call);
+ break;
+ case WITH:
+ result = visitWith((SqlWith) call);
+ break;
+ case WITH_ITEM:
+ result = visitWithItem((SqlWithItem) call);
+ break;
+ // EXPLAIN PLAN FOR is NOT preserved.
+ case EXPLAIN:
+ result = call.getOperandList().get(0).accept(this);
+ break;
+ case CASE:
+ result = visitCase((SqlCase) call);
+ break;
+ case ORDER_BY:
+ result = visitOrderBy((SqlOrderBy) call);
+ break;
+ case OVER:
+ // Window functions: only visit the aggregate function (operand 0).
+ // Skip the window specification (operand 1) due to its complex structure
+ // with ORDER BY and frame clauses. This means literals in PARTITION BY,
+ // ORDER BY, and window frames are preserved rather than replaced.
+ call.setOperand(0, call.getOperandList().get(0).accept(this));
+ result = call;
+ break;
+ case IN:
+ case NOT_IN:
+ result = visitIn(call);
+ break;
+ default:
+ return super.visit(call);
+ }
+ return result;
+ }
+
+ @Nullable
+ private SqlNode visitCase(SqlCase sqlCase) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : sqlCase.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ newOperands.add(child.accept(this));
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ sqlCase.setOperand(i++, operand);
+ }
+ return sqlCase;
+ }
+
+ @Nullable
+ private SqlNode visitSelect(SqlSelect select) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : select.getOperandList()) {
+ newOperands.add(child != null ? child.accept(this) : null);
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // Preserve hints.
+ if (i == SQLSELECT_HINTS_OPERAND_INDEX) {
+ break;
+ }
+ select.setOperand(i++, operand);
+ }
+ return select;
+ }
+
+ @Nullable
+ private SqlNode visitJoin(SqlJoin join) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : join.getOperandList()) {
+ newOperands.add(child != null ? child.accept(this) : null);
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // Preserve join metadata literals:
+ // natural (true/false), joinType (INNER/LEFT/RIGHT/FULL) and conditionType (ON/USING)
+ // These are structural keywords, not data literals.
+ if (i == SQLJOIN_NATURAL_OPERAND_INDEX
+ || i == SQLJOIN_JOIN_TYPE_OPERAND_INDEX
+ || i == SQLJOIN_CONDITION_TYPE_OPERAND_INDEX) {
+ i++;
+ continue;
+ }
+ join.setOperand(i++, operand);
+ }
+ return join;
+ }
+
+ @Nullable
+ private SqlNode visitWith(SqlWith with) {
+ List<SqlNode> newList = new ArrayList<>();
+ for (SqlNode node : with.withList.getList()) {
+ newList.add(node.accept(this));
+ }
+ SqlNode newBody = with.body.accept(this);
+ with.setOperand(0, new SqlNodeList(newList, with.withList.getParserPosition()));
+ with.setOperand(1, newBody);
+ return with;
+ }
+
+ /**
+ * SqlWithItem has four fields: SqlIdentifier name, SqlNodeList
+ * columnList, SqlNode query, and SqlLiteral recursive.
+ * We will visit only the columnList and query since:
+ * - name has already been visited in the SqlWith visit method.
+ * - recursive is a literal which is a property of the WITH item and not the query itself.
+ */
+ @Nullable
+ private SqlNode visitWithItem(SqlWithItem withItem) {
+ if (withItem.columnList != null) {
+ for (int i = 0; i < withItem.columnList.size(); i++) {
+ SqlNode column = withItem.columnList.get(i);
+ if (column != null) {
+ withItem.columnList.set(i, column.accept(this));
+ }
+ }
+ }
+
+ if (withItem.query != null) {
+ SqlNode newQuery = withItem.query.accept(this);
+ withItem.query = newQuery;
+ }
+
+ return withItem;
+ }
+
+ @Nullable
+ private SqlNode visitOrderBy(SqlOrderBy orderBy) {
+ return new SqlOrderBy(
+ orderBy.getParserPosition(),
+ orderBy.query.accept(this),
+ orderBy.orderList,
+ orderBy.offset != null ? orderBy.offset.accept(this) : null,
+ orderBy.fetch != null ? orderBy.fetch.accept(this) : null);
+ }
+
+ /**
+ * Visit IN/NOT IN clause and normalize data literals.
+ * <p>
+ * Two cases:
+ * <ul>
+ * <li><b>Case 1:</b> All values are data literals → replace entire list with a single ?
+ * <br>Example: IN (1, 2, 3) → IN (?)</li>
+ * <li><b>Case 2:</b> Contains any of the following → visit each value individually and replace only data literals:
+ * <ul>
+ * <li>Preserved literals (NULL, DISTINCT, ASC, DESC, etc.): IN (1, NULL, 3) → IN (?, NULL, ?)</li>
Review Comment:
Good point! I've added a squashNullInList option to QueryFingerprintVisitor to address this.
The visitor now has two constructors:
- QueryFingerprintVisitor() - default behavior, preserves NULL (current behavior)
- QueryFingerprintVisitor(boolean squashNullInList) - when true, NULL values in IN-lists are treated as data literals and squashed with other values
Behavior:
- squashNullInList=false (default): IN (1, NULL, 3) → IN (?, NULL, ?)
- squashNullInList=true: IN (1, NULL, 3) → IN (?)
The option can be wired up to a broker config in the future if needed.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on PR #17177:
URL: https://github.com/apache/pinot/pull/17177#issuecomment-3565201954
> @MeihanLi I am out but wanted to say I am very happy to see this getting delivered in open-source finally 😄
Thanks for contributing the original feature at Uber! Happy to contribute this back to OSS on behalf of the Uber Pinot team.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2524723912
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -329,6 +335,22 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
throws Exception {
_queryLogger.log(requestId, query);
+ String queryHash = "";
+ if (_enableQueryFingerprinting) {
+ try {
+ QueryFingerprint queryFingerprint = QueryFingerprintUtils.generateFingerprint(sqlNodeAndOptions);
+ if (queryFingerprint != null) {
+ requestContext.setQueryFingerprint(queryFingerprint);
+ if (queryFingerprint.getQueryHash() != null) {
Review Comment:
We can remove the null checking on queryHash. Both queryHash or fingerprint are guaranteed to be non-null when queryFingerprint is not null.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2602328313
##########
pinot-broker/src/test/java/org/apache/pinot/broker/api/resources/PinotClientRequestTest.java:
##########
@@ -202,4 +203,45 @@ public void testPinotQueryComparison() throws Exception {
assertEquals(comparisonAnalysis.size(), 1);
Assert.assertTrue(comparisonAnalysis.get(0).contains("Mismatch in number of rows returned"));
}
+
+ @Test
+ public void testGetQueryFingerprintSuccess() throws Exception {
+ Request request = mock(Request.class);
+ when(request.getRequestURL()).thenReturn(new StringBuilder());
+
+ // single stage query
+ String requestJson = "{\"sql\": \"SELECT * FROM myTable WHERE id IN (1, 2, 3)\"}";
+ Response response = _pinotClientRequest.getQueryFingerprint(requestJson, request, null);
+
+ assertEquals(response.getStatus(), Response.Status.OK.getStatusCode());
+ QueryFingerprint fingerprint = (QueryFingerprint) response.getEntity();
+ Assert.assertNotNull(fingerprint);
+ Assert.assertNotNull(fingerprint.getQueryHash());
+ Assert.assertNotNull(fingerprint.getFingerprint());
+ Assert.assertEquals(fingerprint.getFingerprint(), "SELECT * FROM `myTable` WHERE `id` IN (?)");
Review Comment:
nit: I would suggest using `org.assertj.core.api.Assertions`, as it generates better error messages by default. Alternatively use copilot or an equivalent tool to generate a description for each assertion to make tests easier to read and/or solve issues when there is an error
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintUtils.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.utils.request;
+
+import com.google.common.hash.Hashing;
+import java.nio.charset.StandardCharsets;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.dialect.AnsiSqlDialect;
+import org.apache.pinot.spi.trace.QueryFingerprint;
+import org.apache.pinot.sql.parsers.SqlNodeAndOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class QueryFingerprintUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryFingerprintUtils.class);
+
+ private QueryFingerprintUtils() {
+ }
+
+ @Nullable
+ public static QueryFingerprint generateFingerprint(SqlNodeAndOptions sqlNodeAndOptions) throws Exception {
+ if (sqlNodeAndOptions == null) {
+ return null;
+ }
+
+ SqlNode sqlNode = sqlNodeAndOptions.getSqlNode();
+ if (sqlNode == null) {
+ return null;
+ }
+
+ QueryFingerprintVisitor visitor = new QueryFingerprintVisitor();
+ SqlNode queryFingerprintNode = sqlNode.accept(visitor);
+
+ if (queryFingerprintNode == null) {
+ return null;
+ }
+
+ String fingerprint = queryFingerprintNode.toSqlString(c -> c.withDialect(AnsiSqlDialect.DEFAULT)).getSql();
+
+ if (fingerprint == null) {
+ return null;
+ }
+
+ // Normalize whitespace: replace newlines with spaces, collapse multiple spaces into one, and trim
+ fingerprint = fingerprint.replace("\n", " ").replaceAll("\\s+", " ").trim();
Review Comment:
nit: I think we can avoid one allocation by doing something like
```
fingerprint = fingerprint.replaceAll("(\\s|\n)+", " ").trim();
```
##########
pinot-tools/src/main/resources/log4j2.xml:
##########
@@ -28,7 +28,7 @@
https://logging.apache.org/log4j/2.x/manual/pattern-layout.html#converter-not-empty
and https://logging.apache.org/log4j/2.x/manual/pattern-layout.html#converter-thread-context-map
-->
- <Property name="LOG_PATTERN">%d{yyyy/MM/dd HH:mm:ss.SSS} %p [%c{1}] [%t]%notEmpty{ [cid=%X{pinot.query.cid}%notEmpty{,stg=%X{pinot.mse.stageId}}]} %m%n</Property>
+ <Property name="LOG_PATTERN">%d{yyyy/MM/dd HH:mm:ss.SSS} %p [%c{1}] [%t]%notEmpty{ [cid=%X{pinot.query.cid}%notEmpty{,queryHash=%X{pinot.query.hash}}%notEmpty{,stg=%X{pinot.mse.stageId}}]} %m%n</Property>
Review Comment:
Same here, can we use a shorter literal than `queryHash`? Maybe `fgp`, just `hash` or `shape`?
Also, this is not a hash of the query but the hash of the fingerprint
##########
pinot-broker/src/main/java/org/apache/pinot/broker/querylog/QueryLogger.java:
##########
@@ -199,6 +200,14 @@ void doFormat(StringBuilder builder, QueryLogger logger, QueryLogParams params)
builder.append(params._table);
}
},
+ QUERY_HASH("queryHash") {
Review Comment:
nit: can we use a shorter term instead of queryHash when logging? In the same way we use `cid` and `stg` instead of `correlationId` and `stage`?
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -167,6 +167,10 @@ public static boolean isCollectGcStats(Map<String, String> queryOptions) {
return Boolean.parseBoolean(queryOptions.get(QueryOptionKey.COLLECT_GC_STATS));
}
+ public static String getQueryHash(Map<String, String> queryOptions) {
+ return queryOptions.getOrDefault(QueryOptionKey.QUERY_HASH, CommonConstants.Broker.DEFAULT_QUERY_HASH);
+ }
Review Comment:
Can we add a broker option to enable this feature by default?
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintVisitor.java:
##########
@@ -0,0 +1,307 @@
+/**
+ * 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.utils.request;
+
+import java.util.ArrayList;
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlJoin;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlOrderBy;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.fun.SqlCase;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+
+/**
+ * QueryFingerprintVisitor traverses the Calcite SqlNode AST and produces a normalized query fingerprint.
+ * Implementation is based on Calcite 1.40.0 version. It may change in future versions of Calcite.
+ *
+ * <p>
+ * <ul>
+ * <li>All data literals are replaced with dynamic parameters (?)</li>
+ * <li>IN/NOT IN clauses with multiple constants are squashed to a single parameter.</li>
+ * <li>EXPLAIN PLAN FOR is NOT preserved.</li>
+ * <li>Symbolic keywords (DISTINCT, ASC, DESC, etc.) and NULL literals are preserved.</li>
+ * <li>Hints are preserved.</li>
+ * <li>Window functions: window specification (operand 1) are preserved.</li>
+ * </ul>
+ * </p>
+ */
+public class QueryFingerprintVisitor extends SqlShuttle {
+ // SqlSelect operand index for hints, see {@link org.apache.calcite.sql.SqlSelect}.
+ private static final int SQLSELECT_HINTS_OPERAND_INDEX = 11;
+
+ // SqlJoin operand indices, see {@link org.apache.calcite.sql.SqlJoin}.
+ private static final int SQLJOIN_NATURAL_OPERAND_INDEX = 1;
+ private static final int SQLJOIN_JOIN_TYPE_OPERAND_INDEX = 2;
+ private static final int SQLJOIN_CONDITION_TYPE_OPERAND_INDEX = 4;
+
+ private int _dynamicParamIndex = 0;
+
+ @Override
+ public SqlNode visit(SqlLiteral literal) {
+ if (shouldPreserveLiteral(literal)) {
+ return literal;
+ }
+ // Replace data literals (numbers, strings, dates, etc.) with dynamic parameters
+ return new SqlDynamicParam(_dynamicParamIndex++, literal.getParserPosition());
+ }
+
+ @Override
+ public @Nullable SqlNode visit(SqlCall call) {
+ SqlNode result = call;
+ switch (call.getKind()) {
+ case SELECT:
+ result = visitSelect((SqlSelect) call);
+ break;
+ case JOIN:
+ result = visitJoin((SqlJoin) call);
+ break;
+ case WITH:
+ result = visitWith((SqlWith) call);
+ break;
+ case WITH_ITEM:
+ result = visitWithItem((SqlWithItem) call);
+ break;
+ // EXPLAIN PLAN FOR is NOT preserved.
+ case EXPLAIN:
+ result = call.getOperandList().get(0).accept(this);
+ break;
+ case CASE:
+ result = visitCase((SqlCase) call);
+ break;
+ case ORDER_BY:
+ result = visitOrderBy((SqlOrderBy) call);
+ break;
+ case OVER:
+ // Window functions: only visit the aggregate function (operand 0).
+ // Skip the window specification (operand 1) due to its complex structure
+ // with ORDER BY and frame clauses. This means literals in PARTITION BY,
+ // ORDER BY, and window frames are preserved rather than replaced.
+ call.setOperand(0, call.getOperandList().get(0).accept(this));
+ result = call;
+ break;
+ case IN:
+ case NOT_IN:
+ result = visitIn(call);
+ break;
+ default:
+ return super.visit(call);
+ }
+ return result;
+ }
+
+ @Nullable
+ private SqlNode visitCase(SqlCase sqlCase) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : sqlCase.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ newOperands.add(child.accept(this));
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ sqlCase.setOperand(i++, operand);
+ }
+ return sqlCase;
+ }
+
+ @Nullable
+ private SqlNode visitSelect(SqlSelect select) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : select.getOperandList()) {
+ newOperands.add(child != null ? child.accept(this) : null);
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // Preserve hints.
+ if (i == SQLSELECT_HINTS_OPERAND_INDEX) {
+ break;
+ }
+ select.setOperand(i++, operand);
+ }
+ return select;
+ }
+
+ @Nullable
+ private SqlNode visitJoin(SqlJoin join) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : join.getOperandList()) {
+ newOperands.add(child != null ? child.accept(this) : null);
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // Preserve join metadata literals:
+ // natural (true/false), joinType (INNER/LEFT/RIGHT/FULL) and conditionType (ON/USING)
+ // These are structural keywords, not data literals.
+ if (i == SQLJOIN_NATURAL_OPERAND_INDEX
+ || i == SQLJOIN_JOIN_TYPE_OPERAND_INDEX
+ || i == SQLJOIN_CONDITION_TYPE_OPERAND_INDEX) {
+ i++;
+ continue;
+ }
+ join.setOperand(i++, operand);
+ }
+ return join;
+ }
+
+ @Nullable
+ private SqlNode visitWith(SqlWith with) {
+ List<SqlNode> newList = new ArrayList<>();
+ for (SqlNode node : with.withList.getList()) {
+ newList.add(node.accept(this));
+ }
+ SqlNode newBody = with.body.accept(this);
+ with.setOperand(0, new SqlNodeList(newList, with.withList.getParserPosition()));
+ with.setOperand(1, newBody);
+ return with;
+ }
+
+ /**
+ * SqlWithItem has four fields: SqlIdentifier name, SqlNodeList
+ * columnList, SqlNode query, and SqlLiteral recursive.
+ * We will visit only the columnList and query since:
+ * - name has already been visited in the SqlWith visit method.
+ * - recursive is a literal which is a property of the WITH item and not the query itself.
+ */
+ @Nullable
+ private SqlNode visitWithItem(SqlWithItem withItem) {
+ if (withItem.columnList != null) {
+ for (int i = 0; i < withItem.columnList.size(); i++) {
+ SqlNode column = withItem.columnList.get(i);
+ if (column != null) {
+ withItem.columnList.set(i, column.accept(this));
+ }
+ }
+ }
+
+ if (withItem.query != null) {
+ SqlNode newQuery = withItem.query.accept(this);
+ withItem.query = newQuery;
+ }
+
+ return withItem;
+ }
+
+ @Nullable
+ private SqlNode visitOrderBy(SqlOrderBy orderBy) {
+ return new SqlOrderBy(
+ orderBy.getParserPosition(),
+ orderBy.query.accept(this),
+ orderBy.orderList,
+ orderBy.offset != null ? orderBy.offset.accept(this) : null,
+ orderBy.fetch != null ? orderBy.fetch.accept(this) : null);
+ }
+
+ /**
+ * Visit IN/NOT IN clause and normalize data literals.
+ * <p>
+ * Two cases:
+ * <ul>
+ * <li><b>Case 1:</b> All values are data literals → replace entire list with a single ?
+ * <br>Example: IN (1, 2, 3) → IN (?)</li>
+ * <li><b>Case 2:</b> Contains any of the following → visit each value individually and replace only data literals:
+ * <ul>
+ * <li>Preserved literals (NULL, DISTINCT, ASC, DESC, etc.): IN (1, NULL, 3) → IN (?, NULL, ?)</li>
Review Comment:
this may create different plans a user creates the list from a bunch of their own values and sometimes generates `IN (1, NULL, 3)` and others `IN (2, 4, NULL)`. I'm fine keeping the code as it is, but probably we will need an option to treat NULL in a list in the same way other literals are treated
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2625665162
##########
pinot-tools/src/main/resources/log4j2.xml:
##########
@@ -28,7 +28,7 @@
https://logging.apache.org/log4j/2.x/manual/pattern-layout.html#converter-not-empty
and https://logging.apache.org/log4j/2.x/manual/pattern-layout.html#converter-thread-context-map
-->
- <Property name="LOG_PATTERN">%d{yyyy/MM/dd HH:mm:ss.SSS} %p [%c{1}] [%t]%notEmpty{ [cid=%X{pinot.query.cid}%notEmpty{,stg=%X{pinot.mse.stageId}}]} %m%n</Property>
+ <Property name="LOG_PATTERN">%d{yyyy/MM/dd HH:mm:ss.SSS} %p [%c{1}] [%t]%notEmpty{ [cid=%X{pinot.query.cid}%notEmpty{,queryHash=%X{pinot.query.hash}}%notEmpty{,stg=%X{pinot.mse.stageId}}]} %m%n</Property>
Review Comment:
updated it to hash
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2625615712
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -297,6 +304,19 @@ protected BrokerResponse handleRequestThrowing(long requestId, String query, Sql
throws QueryException, WebApplicationException {
_queryLogger.log(requestId, query);
+ String queryHash = CommonConstants.Broker.DEFAULT_QUERY_HASH;
+ if (_enableQueryFingerprinting) {
+ try {
+ QueryFingerprint queryFingerprint = QueryFingerprintUtils.generateFingerprint(sqlNodeAndOptions);
+ if (queryFingerprint != null) {
+ requestContext.setQueryFingerprint(queryFingerprint);
+ LoggerConstants.QUERY_HASH_KEY.registerInMdc(queryFingerprint.getQueryHash());
+ }
+ } catch (Exception e) {
+ LOGGER.warn("Failed to generate query fingerprint for request {}: {}. {}", requestId, query, e.getMessage());
+ }
+ }
Review Comment:
fixed in the most recent commit.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2525549396
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintVisitor.java:
##########
@@ -0,0 +1,297 @@
+/**
+ * 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.utils.request;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlAsOperator;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlJoin;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlNumericLiteral;
+import org.apache.calcite.sql.SqlOrderBy;
+import org.apache.calcite.sql.SqlOverOperator;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.fun.SqlCase;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+
+/**
+ * Query fingerprint visitor that traverses the Calcite SqlNode AST and replaces all literals with
+ * dynamic parameters (?), producing a normalized query suitable for fingerprinting.
+ */
+public class QueryFingerprintVisitor extends SqlShuttle {
+ private static final long MISSING_LIMIT_VALUE = Long.MIN_VALUE;
+
+ private boolean _isAlias = false;
+ private boolean _isInFrom = false;
+ private long _maxLimit = MISSING_LIMIT_VALUE;
+
+ /** Use tree-set for storing table names so metrics are emitted in consistent order. */
+ private final Set<String> _tableNames = new TreeSet<>();
+ private final Set<String> _aliases = new HashSet<>();
+ private int _dynamicParamIndex = 0;
+
+ @Override
+ public SqlNode visit(SqlLiteral literal) {
+ // Skip keyword/symbol literals (DISTINCT, NULL, TRUE, FALSE, UNKNOWN, etc.)
+ // These should be preserved in the fingerprint
+ if (literal.getTypeName() == SqlTypeName.SYMBOL
+ || literal.getTypeName() == SqlTypeName.NULL
+ || literal.getTypeName() == SqlTypeName.BOOLEAN) {
+ return literal;
+ }
+ // Replace data literals (numbers, strings, dates, etc.) with dynamic parameters
+ return new SqlDynamicParam(_dynamicParamIndex++, literal.getParserPosition());
+ }
+
+ @Override
+ public @Nullable SqlNode visit(SqlCall call) {
+ SqlNode result = call;
+ switch (call.getKind()) {
+ case SELECT:
+ result = visitSelect((SqlSelect) call);
+ break;
+ case JOIN:
+ result = visitJoin((SqlJoin) call);
+ break;
+ case WITH:
+ result = visitWith((SqlWith) call);
+ break;
+ case WITH_ITEM:
+ result = visitWithItem((SqlWithItem) call);
+ break;
+ case EXPLAIN:
+ result = call.getOperandList().get(0).accept(this);
+ break;
+ case CASE:
+ result = visitCase((SqlCase) call);
+ break;
+ case ORDER_BY:
+ result = visitOrderBy((SqlOrderBy) call);
+ break;
+ // Set operations: UNION, INTERSECT, EXCEPT (all supported by Pinot multi-stage engine)
+ case UNION:
+ case INTERSECT:
+ case EXCEPT:
+ case MINUS:
+ // These are binary operators with two query operands - let default super.visit() handle them
+ return super.visit(call);
+ default:
+ if (call.getOperator() instanceof SqlAsOperator) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ SqlNode firstOperand = call.getOperandList().get(0);
+ SqlNode secondOperand = call.getOperandList().get(1);
+ boolean isAliasBackup = _isAlias;
+ if (firstOperand != null) {
+ newOperands.add(firstOperand.accept(this));
+ } else {
+ newOperands.add(null);
+ }
+ _isAlias = true;
+ if (secondOperand != null) {
+ newOperands.add(secondOperand.accept(this));
+ } else {
+ newOperands.add(null);
+ }
+ _isAlias = isAliasBackup;
+ call.setOperand(0, newOperands.get(0));
+ call.setOperand(1, newOperands.get(1));
+ } else if (call.getOperator() instanceof SqlOverOperator) {
+ // Do not visit the window function
+ // SqlOver operator has 2 operands, the first one is the aggregate function and
+ // the second one is the window.
+ // Window function is whatever is inside OVER() clause. It can be empty () too.
+ // For now, Pinot supports partition by, order by and limited support for window frame.
+ // We can revisit traversing window function if we start finding literals
+ // inside frame clause of window function. Partition by, Order by will not have literal
+ // clauses.
+ call.setOperand(0, call.getOperandList().get(0).accept(this));
+ } else {
+ return super.visit(call);
+ }
+ break;
+ }
+ return result;
+ }
+
+ @Nullable
+ private SqlNode visitCase(SqlCase sqlCase) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : sqlCase.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ newOperands.add(child.accept(this));
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ sqlCase.setOperand(i++, operand);
+ }
+ return sqlCase;
+ }
+
+ @Nullable
+ private SqlNode visitSelect(SqlSelect select) {
+ boolean isInFromBackup = _isInFrom;
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : select.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ _isInFrom = select.getFrom() == child;
+ newOperands.add(child.accept(this));
+ }
+ _isInFrom = isInFromBackup;
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // The 11th node is a `hints` variable of type SqlNodeList, which is more complex to traverse
+ // (as seen in how `columnList` vs `query` is handled in SqlWith). Since SQL hints are
+ // discouraged in Pinot, this was intentionally skipped.
+ if (i == 11) {
+ break;
+ }
+ select.setOperand(i++, operand);
+ }
+ return select;
+ }
+
+ @Nullable
+ private SqlNode visitJoin(SqlJoin join) {
+ boolean isInFromBackup = _isInFrom;
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : join.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ if (child == join.getCondition()) {
+ _isInFrom = false;
+ }
+ newOperands.add(child.accept(this));
+ if (child == join.getCondition()) {
+ _isInFrom = isInFromBackup;
+ }
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // natural must be true or false which is a literal
+ // join type must be a literal from one of the enum values
+ // conditionType must be a literal from one of the enum values
+ if (i == 1 || i == 2 || i == 4) {
+ i++;
+ continue;
+ }
+ join.setOperand(i++, operand);
+ }
+ return join;
+ }
+
+ @Nullable
+ private SqlNode visitWith(SqlWith with) {
+ boolean isInFromBackup = _isInFrom;
+ List<SqlNode> newList = new ArrayList<>();
+ for (SqlNode node : with.withList.getList()) {
+ _isAlias = true;
+ SqlWithItem withItem = (SqlWithItem) node;
+ withItem.name.accept(this);
+ _isAlias = false;
+ newList.add(withItem.accept(this));
+ }
+ SqlNode newBody = with.body.accept(this);
+ with.setOperand(0, new SqlNodeList(newList, with.withList.getParserPosition()));
+ with.setOperand(1, newBody);
+ _isInFrom = isInFromBackup;
+ return with;
+ }
+
+ /**
+ * SqlWithItem has four fields (at the time of writing): SqlIdentifier name, SqlNodeList
Review Comment:
Thanks for catching it. I removed it and added class-level Javadocs to note that the implementation is tied to Calcite 1.40.0.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2524674414
##########
pinot-common/src/main/java/org/apache/pinot/common/request/InstanceRequest.java:
##########
@@ -141,24 +141,24 @@ public java.lang.String getFieldName() {
public static final java.util.Map<_Fields, org.apache.thrift.meta_data.FieldMetaData> metaDataMap;
static {
java.util.Map<_Fields, org.apache.thrift.meta_data.FieldMetaData> tmpMap = new java.util.EnumMap<_Fields, org.apache.thrift.meta_data.FieldMetaData>(_Fields.class);
- tmpMap.put(_Fields.REQUEST_ID, new org.apache.thrift.meta_data.FieldMetaData("requestId", org.apache.thrift.TFieldRequirementType.REQUIRED,
+ tmpMap.put(_Fields.REQUEST_ID, new org.apache.thrift.meta_data.FieldMetaData("requestId", org.apache.thrift.TFieldRequirementType.REQUIRED,
Review Comment:
Thanks for catching this, IDE automatically removed the spaces after commas, reverted them all.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2535284005
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -329,6 +335,22 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
throws Exception {
_queryLogger.log(requestId, query);
+ String queryHash = "";
+ if (_enableQueryFingerprinting) {
+ try {
+ QueryFingerprint queryFingerprint = QueryFingerprintUtils.generateFingerprint(sqlNodeAndOptions);
+ if (queryFingerprint != null) {
+ requestContext.setQueryFingerprint(queryFingerprint);
+ if (queryFingerprint.getQueryHash() != null) {
Review Comment:
@MeihanLi can we add a `Preconditions.check` in the `QueryFingerprint` constructor to assert this?
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2519957899
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintVisitor.java:
##########
@@ -0,0 +1,297 @@
+/**
+ * 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.utils.request;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlAsOperator;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlJoin;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlNumericLiteral;
+import org.apache.calcite.sql.SqlOrderBy;
+import org.apache.calcite.sql.SqlOverOperator;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.fun.SqlCase;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+
+/**
+ * Query fingerprint visitor that traverses the Calcite SqlNode AST and replaces all literals with
+ * dynamic parameters (?), producing a normalized query suitable for fingerprinting.
+ */
+public class QueryFingerprintVisitor extends SqlShuttle {
+ private static final long MISSING_LIMIT_VALUE = Long.MIN_VALUE;
+
+ private boolean _isAlias = false;
+ private boolean _isInFrom = false;
+ private long _maxLimit = MISSING_LIMIT_VALUE;
+
+ /** Use tree-set for storing table names so metrics are emitted in consistent order. */
+ private final Set<String> _tableNames = new TreeSet<>();
+ private final Set<String> _aliases = new HashSet<>();
+ private int _dynamicParamIndex = 0;
+
+ @Override
+ public SqlNode visit(SqlLiteral literal) {
+ // Skip keyword/symbol literals (DISTINCT, NULL, TRUE, FALSE, UNKNOWN, etc.)
+ // These should be preserved in the fingerprint
+ if (literal.getTypeName() == SqlTypeName.SYMBOL
+ || literal.getTypeName() == SqlTypeName.NULL
+ || literal.getTypeName() == SqlTypeName.BOOLEAN) {
+ return literal;
+ }
+ // Replace data literals (numbers, strings, dates, etc.) with dynamic parameters
+ return new SqlDynamicParam(_dynamicParamIndex++, literal.getParserPosition());
+ }
+
+ @Override
+ public @Nullable SqlNode visit(SqlCall call) {
+ SqlNode result = call;
+ switch (call.getKind()) {
+ case SELECT:
+ result = visitSelect((SqlSelect) call);
+ break;
+ case JOIN:
+ result = visitJoin((SqlJoin) call);
+ break;
+ case WITH:
+ result = visitWith((SqlWith) call);
+ break;
+ case WITH_ITEM:
+ result = visitWithItem((SqlWithItem) call);
+ break;
+ case EXPLAIN:
+ result = call.getOperandList().get(0).accept(this);
+ break;
+ case CASE:
+ result = visitCase((SqlCase) call);
+ break;
+ case ORDER_BY:
+ result = visitOrderBy((SqlOrderBy) call);
+ break;
+ // Set operations: UNION, INTERSECT, EXCEPT (all supported by Pinot multi-stage engine)
+ case UNION:
+ case INTERSECT:
+ case EXCEPT:
+ case MINUS:
+ // These are binary operators with two query operands - let default super.visit() handle them
+ return super.visit(call);
+ default:
+ if (call.getOperator() instanceof SqlAsOperator) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ SqlNode firstOperand = call.getOperandList().get(0);
+ SqlNode secondOperand = call.getOperandList().get(1);
+ boolean isAliasBackup = _isAlias;
+ if (firstOperand != null) {
+ newOperands.add(firstOperand.accept(this));
+ } else {
+ newOperands.add(null);
+ }
+ _isAlias = true;
+ if (secondOperand != null) {
+ newOperands.add(secondOperand.accept(this));
+ } else {
+ newOperands.add(null);
+ }
+ _isAlias = isAliasBackup;
+ call.setOperand(0, newOperands.get(0));
+ call.setOperand(1, newOperands.get(1));
+ } else if (call.getOperator() instanceof SqlOverOperator) {
+ // Do not visit the window function
+ // SqlOver operator has 2 operands, the first one is the aggregate function and
+ // the second one is the window.
+ // Window function is whatever is inside OVER() clause. It can be empty () too.
+ // For now, Pinot supports partition by, order by and limited support for window frame.
+ // We can revisit traversing window function if we start finding literals
+ // inside frame clause of window function. Partition by, Order by will not have literal
+ // clauses.
+ call.setOperand(0, call.getOperandList().get(0).accept(this));
+ } else {
+ return super.visit(call);
+ }
+ break;
+ }
+ return result;
+ }
+
+ @Nullable
+ private SqlNode visitCase(SqlCase sqlCase) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : sqlCase.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ newOperands.add(child.accept(this));
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ sqlCase.setOperand(i++, operand);
+ }
+ return sqlCase;
+ }
+
+ @Nullable
+ private SqlNode visitSelect(SqlSelect select) {
+ boolean isInFromBackup = _isInFrom;
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : select.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ _isInFrom = select.getFrom() == child;
+ newOperands.add(child.accept(this));
+ }
+ _isInFrom = isInFromBackup;
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // The 11th node is a `hints` variable of type SqlNodeList, which is more complex to traverse
+ // (as seen in how `columnList` vs `query` is handled in SqlWith). Since SQL hints are
+ // discouraged in Pinot, this was intentionally skipped.
+ if (i == 11) {
+ break;
+ }
+ select.setOperand(i++, operand);
+ }
+ return select;
+ }
+
+ @Nullable
+ private SqlNode visitJoin(SqlJoin join) {
+ boolean isInFromBackup = _isInFrom;
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : join.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ if (child == join.getCondition()) {
+ _isInFrom = false;
+ }
+ newOperands.add(child.accept(this));
+ if (child == join.getCondition()) {
+ _isInFrom = isInFromBackup;
+ }
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // natural must be true or false which is a literal
+ // join type must be a literal from one of the enum values
+ // conditionType must be a literal from one of the enum values
+ if (i == 1 || i == 2 || i == 4) {
+ i++;
+ continue;
+ }
+ join.setOperand(i++, operand);
+ }
+ return join;
+ }
+
+ @Nullable
+ private SqlNode visitWith(SqlWith with) {
+ boolean isInFromBackup = _isInFrom;
+ List<SqlNode> newList = new ArrayList<>();
+ for (SqlNode node : with.withList.getList()) {
+ _isAlias = true;
+ SqlWithItem withItem = (SqlWithItem) node;
+ withItem.name.accept(this);
+ _isAlias = false;
+ newList.add(withItem.accept(this));
+ }
+ SqlNode newBody = with.body.accept(this);
+ with.setOperand(0, new SqlNodeList(newList, with.withList.getParserPosition()));
+ with.setOperand(1, newBody);
+ _isInFrom = isInFromBackup;
+ return with;
+ }
+
+ /**
+ * SqlWithItem has four fields (at the time of writing): SqlIdentifier name, SqlNodeList
Review Comment:
nit: "at the time of writing" -> we can replace that with the Calcite version for more clarity.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintVisitor.java:
##########
@@ -0,0 +1,297 @@
+/**
+ * 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.utils.request;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlAsOperator;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlJoin;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlNumericLiteral;
+import org.apache.calcite.sql.SqlOrderBy;
+import org.apache.calcite.sql.SqlOverOperator;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.fun.SqlCase;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+
+/**
+ * Query fingerprint visitor that traverses the Calcite SqlNode AST and replaces all literals with
+ * dynamic parameters (?), producing a normalized query suitable for fingerprinting.
+ */
+public class QueryFingerprintVisitor extends SqlShuttle {
+ private static final long MISSING_LIMIT_VALUE = Long.MIN_VALUE;
+
+ private boolean _isAlias = false;
+ private boolean _isInFrom = false;
+ private long _maxLimit = MISSING_LIMIT_VALUE;
+
+ /** Use tree-set for storing table names so metrics are emitted in consistent order. */
+ private final Set<String> _tableNames = new TreeSet<>();
+ private final Set<String> _aliases = new HashSet<>();
+ private int _dynamicParamIndex = 0;
+
+ @Override
+ public SqlNode visit(SqlLiteral literal) {
+ // Skip keyword/symbol literals (DISTINCT, NULL, TRUE, FALSE, UNKNOWN, etc.)
+ // These should be preserved in the fingerprint
+ if (literal.getTypeName() == SqlTypeName.SYMBOL
+ || literal.getTypeName() == SqlTypeName.NULL
+ || literal.getTypeName() == SqlTypeName.BOOLEAN) {
+ return literal;
+ }
+ // Replace data literals (numbers, strings, dates, etc.) with dynamic parameters
+ return new SqlDynamicParam(_dynamicParamIndex++, literal.getParserPosition());
+ }
+
+ @Override
+ public @Nullable SqlNode visit(SqlCall call) {
+ SqlNode result = call;
+ switch (call.getKind()) {
+ case SELECT:
+ result = visitSelect((SqlSelect) call);
+ break;
+ case JOIN:
+ result = visitJoin((SqlJoin) call);
+ break;
+ case WITH:
+ result = visitWith((SqlWith) call);
+ break;
+ case WITH_ITEM:
+ result = visitWithItem((SqlWithItem) call);
+ break;
+ case EXPLAIN:
+ result = call.getOperandList().get(0).accept(this);
+ break;
+ case CASE:
+ result = visitCase((SqlCase) call);
+ break;
+ case ORDER_BY:
+ result = visitOrderBy((SqlOrderBy) call);
+ break;
+ // Set operations: UNION, INTERSECT, EXCEPT (all supported by Pinot multi-stage engine)
+ case UNION:
+ case INTERSECT:
+ case EXCEPT:
+ case MINUS:
+ // These are binary operators with two query operands - let default super.visit() handle them
+ return super.visit(call);
+ default:
+ if (call.getOperator() instanceof SqlAsOperator) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ SqlNode firstOperand = call.getOperandList().get(0);
+ SqlNode secondOperand = call.getOperandList().get(1);
+ boolean isAliasBackup = _isAlias;
+ if (firstOperand != null) {
+ newOperands.add(firstOperand.accept(this));
+ } else {
+ newOperands.add(null);
+ }
+ _isAlias = true;
+ if (secondOperand != null) {
+ newOperands.add(secondOperand.accept(this));
+ } else {
+ newOperands.add(null);
+ }
+ _isAlias = isAliasBackup;
+ call.setOperand(0, newOperands.get(0));
+ call.setOperand(1, newOperands.get(1));
+ } else if (call.getOperator() instanceof SqlOverOperator) {
+ // Do not visit the window function
+ // SqlOver operator has 2 operands, the first one is the aggregate function and
+ // the second one is the window.
+ // Window function is whatever is inside OVER() clause. It can be empty () too.
+ // For now, Pinot supports partition by, order by and limited support for window frame.
+ // We can revisit traversing window function if we start finding literals
+ // inside frame clause of window function. Partition by, Order by will not have literal
+ // clauses.
+ call.setOperand(0, call.getOperandList().get(0).accept(this));
+ } else {
+ return super.visit(call);
+ }
+ break;
+ }
+ return result;
+ }
+
+ @Nullable
+ private SqlNode visitCase(SqlCase sqlCase) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : sqlCase.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ newOperands.add(child.accept(this));
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ sqlCase.setOperand(i++, operand);
+ }
+ return sqlCase;
+ }
+
+ @Nullable
+ private SqlNode visitSelect(SqlSelect select) {
+ boolean isInFromBackup = _isInFrom;
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : select.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ _isInFrom = select.getFrom() == child;
+ newOperands.add(child.accept(this));
+ }
+ _isInFrom = isInFromBackup;
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // The 11th node is a `hints` variable of type SqlNodeList, which is more complex to traverse
+ // (as seen in how `columnList` vs `query` is handled in SqlWith). Since SQL hints are
+ // discouraged in Pinot, this was intentionally skipped.
+ if (i == 11) {
Review Comment:
We should explicit public static constants defined for each index we use for comparison, so that with future Calcite updates, it is easy to find the index of the field we are interested in and replace.
##########
pinot-common/src/main/java/org/apache/pinot/common/request/InstanceRequest.java:
##########
@@ -141,24 +141,24 @@ public java.lang.String getFieldName() {
public static final java.util.Map<_Fields, org.apache.thrift.meta_data.FieldMetaData> metaDataMap;
static {
java.util.Map<_Fields, org.apache.thrift.meta_data.FieldMetaData> tmpMap = new java.util.EnumMap<_Fields, org.apache.thrift.meta_data.FieldMetaData>(_Fields.class);
- tmpMap.put(_Fields.REQUEST_ID, new org.apache.thrift.meta_data.FieldMetaData("requestId", org.apache.thrift.TFieldRequirementType.REQUIRED,
+ tmpMap.put(_Fields.REQUEST_ID, new org.apache.thrift.meta_data.FieldMetaData("requestId", org.apache.thrift.TFieldRequirementType.REQUIRED,
Review Comment:
Can we revert these changes? Looks like spacing changes.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -329,6 +335,22 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
throws Exception {
_queryLogger.log(requestId, query);
+ String queryHash = "";
+ if (_enableQueryFingerprinting) {
+ try {
+ QueryFingerprint queryFingerprint = QueryFingerprintUtils.generateFingerprint(sqlNodeAndOptions);
+ if (queryFingerprint != null) {
+ requestContext.setQueryFingerprint(queryFingerprint);
+ if (queryFingerprint.getQueryHash() != null) {
Review Comment:
Do we envision a case where the `queryFingerprint` object is not `null` and the query hash within it is `null`?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/query/QueryExecutionContext.java:
##########
@@ -104,20 +107,21 @@ public static QueryExecutionContext forTseServerRequest(Map<String, String> requ
String workloadName = requestMetadata.getOrDefault(QueryOptionKey.WORKLOAD_NAME, Accounting.DEFAULT_WORKLOAD_NAME);
long deadlineMs = Long.parseLong(requestMetadata.get(TimeSeries.DEADLINE_MS));
String brokerId = requestMetadata.getOrDefault(MetadataKeys.BROKER_ID, "unknown");
+ String queryHash = requestMetadata.getOrDefault(QueryOptionKey.QUERY_HASH, "");
return new QueryExecutionContext(QueryType.TSE, requestId, cid, workloadName, startTimeMs, deadlineMs, deadlineMs,
- brokerId, instanceId);
+ brokerId, instanceId, queryHash);
}
@VisibleForTesting
public static QueryExecutionContext forSseTest() {
return new QueryExecutionContext(QueryType.SSE, 123L, "cid", Accounting.DEFAULT_WORKLOAD_NAME,
- System.currentTimeMillis(), Long.MAX_VALUE, Long.MAX_VALUE, "brokerId", "instanceId");
+ System.currentTimeMillis(), Long.MAX_VALUE, Long.MAX_VALUE, "brokerId", "instanceId", "");
Review Comment:
nit: We can use the DEFAULT_QUERY_HASH const here.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintVisitor.java:
##########
@@ -0,0 +1,297 @@
+/**
+ * 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.utils.request;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlAsOperator;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlJoin;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlNumericLiteral;
+import org.apache.calcite.sql.SqlOrderBy;
+import org.apache.calcite.sql.SqlOverOperator;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.fun.SqlCase;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+
+/**
+ * Query fingerprint visitor that traverses the Calcite SqlNode AST and replaces all literals with
+ * dynamic parameters (?), producing a normalized query suitable for fingerprinting.
+ */
+public class QueryFingerprintVisitor extends SqlShuttle {
+ private static final long MISSING_LIMIT_VALUE = Long.MIN_VALUE;
+
+ private boolean _isAlias = false;
+ private boolean _isInFrom = false;
+ private long _maxLimit = MISSING_LIMIT_VALUE;
+
+ /** Use tree-set for storing table names so metrics are emitted in consistent order. */
+ private final Set<String> _tableNames = new TreeSet<>();
+ private final Set<String> _aliases = new HashSet<>();
+ private int _dynamicParamIndex = 0;
+
+ @Override
+ public SqlNode visit(SqlLiteral literal) {
+ // Skip keyword/symbol literals (DISTINCT, NULL, TRUE, FALSE, UNKNOWN, etc.)
+ // These should be preserved in the fingerprint
+ if (literal.getTypeName() == SqlTypeName.SYMBOL
+ || literal.getTypeName() == SqlTypeName.NULL
+ || literal.getTypeName() == SqlTypeName.BOOLEAN) {
+ return literal;
+ }
+ // Replace data literals (numbers, strings, dates, etc.) with dynamic parameters
+ return new SqlDynamicParam(_dynamicParamIndex++, literal.getParserPosition());
+ }
+
+ @Override
+ public @Nullable SqlNode visit(SqlCall call) {
+ SqlNode result = call;
+ switch (call.getKind()) {
+ case SELECT:
+ result = visitSelect((SqlSelect) call);
+ break;
+ case JOIN:
+ result = visitJoin((SqlJoin) call);
+ break;
+ case WITH:
+ result = visitWith((SqlWith) call);
+ break;
+ case WITH_ITEM:
+ result = visitWithItem((SqlWithItem) call);
+ break;
+ case EXPLAIN:
+ result = call.getOperandList().get(0).accept(this);
+ break;
+ case CASE:
+ result = visitCase((SqlCase) call);
+ break;
+ case ORDER_BY:
+ result = visitOrderBy((SqlOrderBy) call);
+ break;
+ // Set operations: UNION, INTERSECT, EXCEPT (all supported by Pinot multi-stage engine)
+ case UNION:
+ case INTERSECT:
+ case EXCEPT:
+ case MINUS:
+ // These are binary operators with two query operands - let default super.visit() handle them
+ return super.visit(call);
+ default:
+ if (call.getOperator() instanceof SqlAsOperator) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ SqlNode firstOperand = call.getOperandList().get(0);
+ SqlNode secondOperand = call.getOperandList().get(1);
+ boolean isAliasBackup = _isAlias;
+ if (firstOperand != null) {
+ newOperands.add(firstOperand.accept(this));
+ } else {
+ newOperands.add(null);
+ }
+ _isAlias = true;
+ if (secondOperand != null) {
+ newOperands.add(secondOperand.accept(this));
+ } else {
+ newOperands.add(null);
+ }
+ _isAlias = isAliasBackup;
+ call.setOperand(0, newOperands.get(0));
+ call.setOperand(1, newOperands.get(1));
+ } else if (call.getOperator() instanceof SqlOverOperator) {
+ // Do not visit the window function
+ // SqlOver operator has 2 operands, the first one is the aggregate function and
+ // the second one is the window.
+ // Window function is whatever is inside OVER() clause. It can be empty () too.
+ // For now, Pinot supports partition by, order by and limited support for window frame.
+ // We can revisit traversing window function if we start finding literals
+ // inside frame clause of window function. Partition by, Order by will not have literal
+ // clauses.
+ call.setOperand(0, call.getOperandList().get(0).accept(this));
+ } else {
+ return super.visit(call);
+ }
+ break;
+ }
+ return result;
+ }
+
+ @Nullable
+ private SqlNode visitCase(SqlCase sqlCase) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : sqlCase.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ newOperands.add(child.accept(this));
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ sqlCase.setOperand(i++, operand);
+ }
+ return sqlCase;
+ }
+
+ @Nullable
+ private SqlNode visitSelect(SqlSelect select) {
+ boolean isInFromBackup = _isInFrom;
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : select.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ _isInFrom = select.getFrom() == child;
+ newOperands.add(child.accept(this));
+ }
+ _isInFrom = isInFromBackup;
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // The 11th node is a `hints` variable of type SqlNodeList, which is more complex to traverse
+ // (as seen in how `columnList` vs `query` is handled in SqlWith). Since SQL hints are
+ // discouraged in Pinot, this was intentionally skipped.
+ if (i == 11) {
+ break;
+ }
+ select.setOperand(i++, operand);
+ }
+ return select;
+ }
+
+ @Nullable
+ private SqlNode visitJoin(SqlJoin join) {
+ boolean isInFromBackup = _isInFrom;
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : join.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ if (child == join.getCondition()) {
+ _isInFrom = false;
+ }
+ newOperands.add(child.accept(this));
+ if (child == join.getCondition()) {
+ _isInFrom = isInFromBackup;
+ }
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // natural must be true or false which is a literal
+ // join type must be a literal from one of the enum values
+ // conditionType must be a literal from one of the enum values
+ if (i == 1 || i == 2 || i == 4) {
Review Comment:
Same here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2625646330
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintUtils.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.utils.request;
+
+import com.google.common.hash.Hashing;
+import java.nio.charset.StandardCharsets;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.dialect.AnsiSqlDialect;
+import org.apache.pinot.spi.trace.QueryFingerprint;
+import org.apache.pinot.sql.parsers.SqlNodeAndOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class QueryFingerprintUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryFingerprintUtils.class);
+
+ private QueryFingerprintUtils() {
+ }
+
+ @Nullable
+ public static QueryFingerprint generateFingerprint(SqlNodeAndOptions sqlNodeAndOptions) throws Exception {
+ if (sqlNodeAndOptions == null) {
+ return null;
+ }
+
+ SqlNode sqlNode = sqlNodeAndOptions.getSqlNode();
+ if (sqlNode == null) {
+ return null;
+ }
+
+ QueryFingerprintVisitor visitor = new QueryFingerprintVisitor();
+ SqlNode queryFingerprintNode = sqlNode.accept(visitor);
+
+ if (queryFingerprintNode == null) {
+ return null;
+ }
+
+ String fingerprint = queryFingerprintNode.toSqlString(c -> c.withDialect(AnsiSqlDialect.DEFAULT)).getSql();
+
+ if (fingerprint == null) {
+ return null;
+ }
+
+ // Normalize whitespace: replace newlines with spaces, collapse multiple spaces into one, and trim
+ fingerprint = fingerprint.replace("\n", " ").replaceAll("\\s+", " ").trim();
Review Comment:
Thanks for catching it. updated to fingerprint = fingerprint.replaceAll("\\s+", " ").trim();
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on PR #17177:
URL: https://github.com/apache/pinot/pull/17177#issuecomment-3663741638
> Can you resolve the conflicts?
Thanks @gortiz , addressed all open comments from you and Copilot and rebased
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2599796959
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -297,6 +304,19 @@ protected BrokerResponse handleRequestThrowing(long requestId, String query, Sql
throws QueryException, WebApplicationException {
_queryLogger.log(requestId, query);
+ String queryHash = CommonConstants.Broker.DEFAULT_QUERY_HASH;
+ if (_enableQueryFingerprinting) {
+ try {
+ QueryFingerprint queryFingerprint = QueryFingerprintUtils.generateFingerprint(sqlNodeAndOptions);
+ if (queryFingerprint != null) {
+ requestContext.setQueryFingerprint(queryFingerprint);
+ LoggerConstants.QUERY_HASH_KEY.registerInMdc(queryFingerprint.getQueryHash());
+ }
+ } catch (Exception e) {
+ LOGGER.warn("Failed to generate query fingerprint for request {}: {}. {}", requestId, query, e.getMessage());
+ }
+ }
Review Comment:
The `queryHash` variable is initialized to `DEFAULT_QUERY_HASH` but never updated when fingerprinting succeeds. The variable should be set to `queryFingerprint.getQueryHash()` when the fingerprint is successfully generated so it can be passed to the `QueryExecutionContext` constructor on line 334. Without this, the queryHash will always be empty even when fingerprinting is enabled.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/query/QueryThreadContext.java:
##########
@@ -129,6 +133,9 @@ public void close() {
THREAD_LOCAL.remove();
LoggerConstants.REQUEST_ID_KEY.unregisterFromMdc();
LoggerConstants.CORRELATION_ID_KEY.unregisterFromMdc();
+ if (_executionContext.getQueryHash() != null) {
+ LoggerConstants.QUERY_HASH_KEY.unregisterFromMdc();
+ }
Review Comment:
The MDC cleanup logic is inconsistent with registration. The registration on line 69 checks `if (queryHash != null)` before registering, but unregistration checks `if (_executionContext.getQueryHash() != null)`. If `getQueryHash()` returns an empty string (the default), it will be non-null and registered in MDC, but this cleanup check will still pass and attempt to unregister. Consider checking for empty string as well, or better yet, always unregister unconditionally since `MDC.remove()` is safe to call even if the key doesn't exist.
```suggestion
LoggerConstants.QUERY_HASH_KEY.unregisterFromMdc();
```
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintUtils.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.utils.request;
+
+import com.google.common.hash.Hashing;
+import java.nio.charset.StandardCharsets;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.dialect.AnsiSqlDialect;
+import org.apache.pinot.spi.trace.QueryFingerprint;
+import org.apache.pinot.sql.parsers.SqlNodeAndOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class QueryFingerprintUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryFingerprintUtils.class);
+
+ private QueryFingerprintUtils() {
+ }
+
+ @Nullable
+ public static QueryFingerprint generateFingerprint(SqlNodeAndOptions sqlNodeAndOptions) throws Exception {
+ if (sqlNodeAndOptions == null) {
+ return null;
+ }
+
+ SqlNode sqlNode = sqlNodeAndOptions.getSqlNode();
+ if (sqlNode == null) {
+ return null;
+ }
+
+ QueryFingerprintVisitor visitor = new QueryFingerprintVisitor();
+ SqlNode queryFingerprintNode = sqlNode.accept(visitor);
+
+ if (queryFingerprintNode == null) {
+ return null;
+ }
+
+ String fingerprint = queryFingerprintNode.toSqlString(c -> c.withDialect(AnsiSqlDialect.DEFAULT)).getSql();
+
+ if (fingerprint == null) {
+ return null;
+ }
+
+ // Normalize whitespace: replace newlines with spaces, collapse multiple spaces into one, and trim
+ fingerprint = fingerprint.replace("\n", " ").replaceAll("\\s+", " ").trim();
+
+ return new QueryFingerprint(hashString(fingerprint), fingerprint);
+ }
+
+ private static String hashString(String input) {
+ return Hashing.farmHashFingerprint64()
+ .hashString(input, StandardCharsets.UTF_8)
+ .toString();
+ }
Review Comment:
The `hashString` method uses `farmHashFingerprint64()` which produces a 64-bit hash, but the PR description states that SHA-256 is used. This inconsistency between the documentation and implementation should be corrected. Either update the implementation to use SHA-256 as documented, or update the PR description to reflect that FarmHash is being used.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -329,6 +335,19 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
throws Exception {
_queryLogger.log(requestId, query);
+ String queryHash = CommonConstants.Broker.DEFAULT_QUERY_HASH;
+ if (_enableQueryFingerprinting) {
+ try {
+ QueryFingerprint queryFingerprint = QueryFingerprintUtils.generateFingerprint(sqlNodeAndOptions);
+ if (queryFingerprint != null) {
+ requestContext.setQueryFingerprint(queryFingerprint);
+ LoggerConstants.QUERY_HASH_KEY.registerInMdc(queryFingerprint.getQueryHash());
+ }
+ } catch (Exception e) {
+ LOGGER.warn("Failed to generate query fingerprint for request {}: {}. {}", requestId, query, e.getMessage());
+ }
+ }
Review Comment:
The `queryHash` variable is initialized to `DEFAULT_QUERY_HASH` but never updated when fingerprinting succeeds. The variable should be set to `queryFingerprint.getQueryHash()` when the fingerprint is successfully generated so it can be passed to the `QueryExecutionContext` constructor on line 362. This is the same issue as in `MultiStageBrokerRequestHandler`.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "kishoreg (via GitHub)" <gi...@apache.org>.
kishoreg commented on PR #17177:
URL: https://github.com/apache/pinot/pull/17177#issuecomment-3544572565
> ```
> "SELECT col1 FROM table1 WHERE col2 IN (100, 200, 300)"
> => "SELECT `col1` FROM `table1` WHERE `col2` IN (?, ?, ?)"
> "SELECT col1 FROM table1 WHERE col2 IN (SELECT col2 FROM table2 WHERE col3 = 100)"
> => "SELECT `col1` FROM `table1` WHERE `col2` IN (SELECT `col2` FROM `table2` WHERE `col3` = ?)"
> ```
Can we generate only one ? for 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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "kishoreg (via GitHub)" <gi...@apache.org>.
kishoreg commented on PR #17177:
URL: https://github.com/apache/pinot/pull/17177#issuecomment-3543587418
This is a great feature. Do we have some sample queries and the fingerprint that gets created? I am particularly interested in IN clause etc.
Also, can we extend this to support prepared statements and avoid planning if the query fingerprint matches. Basically, cache the plan per fingerprint and re use the cached plan
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "kishoreg (via GitHub)" <gi...@apache.org>.
kishoreg commented on PR #17177:
URL: https://github.com/apache/pinot/pull/17177#issuecomment-3565327579
Thank you. If it’s not too much work, Can you add a rest endpoint if it’s not added. This can allow us to build tooling for AI use cases and also testing
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2525551402
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintVisitor.java:
##########
@@ -0,0 +1,297 @@
+/**
+ * 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.utils.request;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlAsOperator;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlJoin;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlNumericLiteral;
+import org.apache.calcite.sql.SqlOrderBy;
+import org.apache.calcite.sql.SqlOverOperator;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.fun.SqlCase;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+
+/**
+ * Query fingerprint visitor that traverses the Calcite SqlNode AST and replaces all literals with
+ * dynamic parameters (?), producing a normalized query suitable for fingerprinting.
+ */
+public class QueryFingerprintVisitor extends SqlShuttle {
+ private static final long MISSING_LIMIT_VALUE = Long.MIN_VALUE;
+
+ private boolean _isAlias = false;
+ private boolean _isInFrom = false;
+ private long _maxLimit = MISSING_LIMIT_VALUE;
+
+ /** Use tree-set for storing table names so metrics are emitted in consistent order. */
+ private final Set<String> _tableNames = new TreeSet<>();
+ private final Set<String> _aliases = new HashSet<>();
+ private int _dynamicParamIndex = 0;
+
+ @Override
+ public SqlNode visit(SqlLiteral literal) {
+ // Skip keyword/symbol literals (DISTINCT, NULL, TRUE, FALSE, UNKNOWN, etc.)
+ // These should be preserved in the fingerprint
+ if (literal.getTypeName() == SqlTypeName.SYMBOL
+ || literal.getTypeName() == SqlTypeName.NULL
+ || literal.getTypeName() == SqlTypeName.BOOLEAN) {
+ return literal;
+ }
+ // Replace data literals (numbers, strings, dates, etc.) with dynamic parameters
+ return new SqlDynamicParam(_dynamicParamIndex++, literal.getParserPosition());
+ }
+
+ @Override
+ public @Nullable SqlNode visit(SqlCall call) {
+ SqlNode result = call;
+ switch (call.getKind()) {
+ case SELECT:
+ result = visitSelect((SqlSelect) call);
+ break;
+ case JOIN:
+ result = visitJoin((SqlJoin) call);
+ break;
+ case WITH:
+ result = visitWith((SqlWith) call);
+ break;
+ case WITH_ITEM:
+ result = visitWithItem((SqlWithItem) call);
+ break;
+ case EXPLAIN:
+ result = call.getOperandList().get(0).accept(this);
+ break;
+ case CASE:
+ result = visitCase((SqlCase) call);
+ break;
+ case ORDER_BY:
+ result = visitOrderBy((SqlOrderBy) call);
+ break;
+ // Set operations: UNION, INTERSECT, EXCEPT (all supported by Pinot multi-stage engine)
+ case UNION:
+ case INTERSECT:
+ case EXCEPT:
+ case MINUS:
+ // These are binary operators with two query operands - let default super.visit() handle them
+ return super.visit(call);
+ default:
+ if (call.getOperator() instanceof SqlAsOperator) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ SqlNode firstOperand = call.getOperandList().get(0);
+ SqlNode secondOperand = call.getOperandList().get(1);
+ boolean isAliasBackup = _isAlias;
+ if (firstOperand != null) {
+ newOperands.add(firstOperand.accept(this));
+ } else {
+ newOperands.add(null);
+ }
+ _isAlias = true;
+ if (secondOperand != null) {
+ newOperands.add(secondOperand.accept(this));
+ } else {
+ newOperands.add(null);
+ }
+ _isAlias = isAliasBackup;
+ call.setOperand(0, newOperands.get(0));
+ call.setOperand(1, newOperands.get(1));
+ } else if (call.getOperator() instanceof SqlOverOperator) {
+ // Do not visit the window function
+ // SqlOver operator has 2 operands, the first one is the aggregate function and
+ // the second one is the window.
+ // Window function is whatever is inside OVER() clause. It can be empty () too.
+ // For now, Pinot supports partition by, order by and limited support for window frame.
+ // We can revisit traversing window function if we start finding literals
+ // inside frame clause of window function. Partition by, Order by will not have literal
+ // clauses.
+ call.setOperand(0, call.getOperandList().get(0).accept(this));
+ } else {
+ return super.visit(call);
+ }
+ break;
+ }
+ return result;
+ }
+
+ @Nullable
+ private SqlNode visitCase(SqlCase sqlCase) {
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : sqlCase.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ newOperands.add(child.accept(this));
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ sqlCase.setOperand(i++, operand);
+ }
+ return sqlCase;
+ }
+
+ @Nullable
+ private SqlNode visitSelect(SqlSelect select) {
+ boolean isInFromBackup = _isInFrom;
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : select.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ _isInFrom = select.getFrom() == child;
+ newOperands.add(child.accept(this));
+ }
+ _isInFrom = isInFromBackup;
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // The 11th node is a `hints` variable of type SqlNodeList, which is more complex to traverse
+ // (as seen in how `columnList` vs `query` is handled in SqlWith). Since SQL hints are
+ // discouraged in Pinot, this was intentionally skipped.
+ if (i == 11) {
+ break;
+ }
+ select.setOperand(i++, operand);
+ }
+ return select;
+ }
+
+ @Nullable
+ private SqlNode visitJoin(SqlJoin join) {
+ boolean isInFromBackup = _isInFrom;
+ List<SqlNode> newOperands = new ArrayList<>();
+ for (SqlNode child : join.getOperandList()) {
+ if (child == null) {
+ newOperands.add(null);
+ continue;
+ }
+ if (child == join.getCondition()) {
+ _isInFrom = false;
+ }
+ newOperands.add(child.accept(this));
+ if (child == join.getCondition()) {
+ _isInFrom = isInFromBackup;
+ }
+ }
+ int i = 0;
+ for (SqlNode operand : newOperands) {
+ // natural must be true or false which is a literal
+ // join type must be a literal from one of the enum values
+ // conditionType must be a literal from one of the enum values
+ if (i == 1 || i == 2 || i == 4) {
Review Comment:
Addressed. Added public static constants with Javadocs.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2624684162
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -167,6 +167,10 @@ public static boolean isCollectGcStats(Map<String, String> queryOptions) {
return Boolean.parseBoolean(queryOptions.get(QueryOptionKey.COLLECT_GC_STATS));
}
+ public static String getQueryHash(Map<String, String> queryOptions) {
+ return queryOptions.getOrDefault(QueryOptionKey.QUERY_HASH, CommonConstants.Broker.DEFAULT_QUERY_HASH);
+ }
Review Comment:
There is already a broker-level option to enable it by default. Check CommonConstants.java, we can enable it on broker by setting `pinot.broker.enable.query.fingerprinting = true`.
There is no query-level option to enable it. If there is a need, we can add later.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2624684162
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -167,6 +167,10 @@ public static boolean isCollectGcStats(Map<String, String> queryOptions) {
return Boolean.parseBoolean(queryOptions.get(QueryOptionKey.COLLECT_GC_STATS));
}
+ public static String getQueryHash(Map<String, String> queryOptions) {
+ return queryOptions.getOrDefault(QueryOptionKey.QUERY_HASH, CommonConstants.Broker.DEFAULT_QUERY_HASH);
+ }
Review Comment:
There is already a broker-level option to enable it by default. Check CommonConstants.java, we can enable it on broker by setting `pinot.broker.enable.query.fingerprinting = true`.
There is no query-level option to enable 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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17177:
URL: https://github.com/apache/pinot/pull/17177#issuecomment-3513542026
### :x: 9 Tests Failed:
| Tests completed | Failed | Passed | Skipped |
|---|---|---|---|
| 8605 | 9 | 8596 | 19 |
<details><summary>View the top 3 failed test(s) by shortest run time</summary>
> <pre><code class="language-python">org.apache.pinot.spi.trace.QueryFingerprintTest::testWithNullFingerprint</code></pre>
> <details><summary>Stack Traces | 0s run time</summary>
>
> > <pre><code class="language-python">expected [null] but found [abc123]</code></pre>
>
> </details>
> <pre><code class="language-python">org.apache.pinot.spi.trace.QueryFingerprintTest::testWithNullFingerprint</code></pre>
> <details><summary>Stack Traces | 0s run time</summary>
>
> > <pre><code class="language-python">expected [null] but found [abc123]</code></pre>
>
> </details>
> <pre><code class="language-python">org.apache.pinot.spi.trace.QueryFingerprintTest::testWithNullFingerprint</code></pre>
> <details><summary>Stack Traces | 0s run time</summary>
>
> > <pre><code class="language-python">expected [null] but found [abc123]</code></pre>
>
> </details>
> <pre><code class="language-python">org.apache.pinot.spi.trace.QueryFingerprintTest::testWithNullQueryHash</code></pre>
> <details><summary>Stack Traces | 0s run time</summary>
>
> > <pre><code class="language-python">expected [SELECT * FROM table] but found [null]</code></pre>
>
> </details>
> <pre><code class="language-python">org.apache.pinot.spi.trace.QueryFingerprintTest::testWithNullQueryHash</code></pre>
> <details><summary>Stack Traces | 0s run time</summary>
>
> > <pre><code class="language-python">expected [SELECT * FROM table] but found [null]</code></pre>
>
> </details>
> <pre><code class="language-python">org.apache.pinot.spi.trace.QueryFingerprintTest::testWithNullQueryHash</code></pre>
> <details><summary>Stack Traces | 0.001s run time</summary>
>
> > <pre><code class="language-python">expected [SELECT * FROM table] but found [null]</code></pre>
>
> </details>
> <pre><code class="language-python">org.apache.pinot.spi.trace.QueryFingerprintTest::testConstructorAndGetters</code></pre>
> <details><summary>Stack Traces | 0.01s run time</summary>
>
> > <pre><code class="language-python">expected [SELECT col1 FROM myTable WHERE col2 = ?] but found [abc123def456]</code></pre>
>
> </details>
> <pre><code class="language-python">org.apache.pinot.spi.trace.QueryFingerprintTest::testConstructorAndGetters</code></pre>
> <details><summary>Stack Traces | 0.01s run time</summary>
>
> > <pre><code class="language-python">expected [SELECT col1 FROM myTable WHERE col2 = ?] but found [abc123def456]</code></pre>
>
> </details>
> <pre><code class="language-python">org.apache.pinot.spi.trace.QueryFingerprintTest::testConstructorAndGetters</code></pre>
> <details><summary>Stack Traces | 0.014s run time</summary>
>
> > <pre><code class="language-python">expected [SELECT col1 FROM myTable WHERE col2 = ?] but found [abc123def456]</code></pre>
>
> </details>
</details>
To view more test analytics, go to the [Test Analytics Dashboard](https://app.codecov.io/gh/apache/pinot/tests/MeihanLi%3AqueryFingerprint)
<sub>📋 Got 3 mins? [Take this short survey](https://forms.gle/22i53Qa1CySZjA6c7) to help us improve Test Analytics.</sub>
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #17177:
URL: https://github.com/apache/pinot/pull/17177#issuecomment-3654865957
Can you resolve the conflicts?
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on PR #17177:
URL: https://github.com/apache/pinot/pull/17177#issuecomment-3565207606
Tested locally with single-stage and multi-stage queries with the feature enabled and disabled. Please check the Test section for results.
Next step is to add the queryHash field to the Pinot broker response, propagating the query fingerprint to clients for end-to-end monitoring.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on PR #17177:
URL: https://github.com/apache/pinot/pull/17177#issuecomment-3572994948
> Thank you. If it’s not too much work, Can you add a rest endpoint if it’s not added. This can allow us to build tooling for AI use cases and also testing
Sure, added broker POST query/sql/queryFingerprint endpoint, please check above Test section for details.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2625654823
##########
pinot-broker/src/test/java/org/apache/pinot/broker/api/resources/PinotClientRequestTest.java:
##########
@@ -202,4 +203,45 @@ public void testPinotQueryComparison() throws Exception {
assertEquals(comparisonAnalysis.size(), 1);
Assert.assertTrue(comparisonAnalysis.get(0).contains("Mismatch in number of rows returned"));
}
+
+ @Test
+ public void testGetQueryFingerprintSuccess() throws Exception {
+ Request request = mock(Request.class);
+ when(request.getRequestURL()).thenReturn(new StringBuilder());
+
+ // single stage query
+ String requestJson = "{\"sql\": \"SELECT * FROM myTable WHERE id IN (1, 2, 3)\"}";
+ Response response = _pinotClientRequest.getQueryFingerprint(requestJson, request, null);
+
+ assertEquals(response.getStatus(), Response.Status.OK.getStatusCode());
+ QueryFingerprint fingerprint = (QueryFingerprint) response.getEntity();
+ Assert.assertNotNull(fingerprint);
+ Assert.assertNotNull(fingerprint.getQueryHash());
+ Assert.assertNotNull(fingerprint.getFingerprint());
+ Assert.assertEquals(fingerprint.getFingerprint(), "SELECT * FROM `myTable` WHERE `id` IN (?)");
Review Comment:
I added some descriptive messages for each assertion statement, which can address your concern. org.assertj.core.api.Assertions is not included as a dependency in this module, and I'd rather keep it consistent with the current UT behavior.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2625666630
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -329,6 +335,19 @@ protected BrokerResponse handleRequest(long requestId, String query, SqlNodeAndO
throws Exception {
_queryLogger.log(requestId, query);
+ String queryHash = CommonConstants.Broker.DEFAULT_QUERY_HASH;
+ if (_enableQueryFingerprinting) {
+ try {
+ QueryFingerprint queryFingerprint = QueryFingerprintUtils.generateFingerprint(sqlNodeAndOptions);
+ if (queryFingerprint != null) {
+ requestContext.setQueryFingerprint(queryFingerprint);
+ LoggerConstants.QUERY_HASH_KEY.registerInMdc(queryFingerprint.getQueryHash());
+ }
+ } catch (Exception e) {
+ LOGGER.warn("Failed to generate query fingerprint for request {}: {}. {}", requestId, query, e.getMessage());
+ }
+ }
Review Comment:
Fixed in the most recent commit.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2625657930
##########
pinot-broker/src/main/java/org/apache/pinot/broker/querylog/QueryLogger.java:
##########
@@ -199,6 +200,14 @@ void doFormat(StringBuilder builder, QueryLogger logger, QueryLogParams params)
builder.append(params._table);
}
},
+ QUERY_HASH("queryHash") {
Review Comment:
yes, the change is in ~/pinot/pinot-tools/src/main/resources/log4j2.xml, we use `hash` now
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2624456826
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintUtils.java:
##########
@@ -0,0 +1,72 @@
+/**
+ * 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.utils.request;
+
+import com.google.common.hash.Hashing;
+import java.nio.charset.StandardCharsets;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.dialect.AnsiSqlDialect;
+import org.apache.pinot.spi.trace.QueryFingerprint;
+import org.apache.pinot.sql.parsers.SqlNodeAndOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class QueryFingerprintUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryFingerprintUtils.class);
+
+ private QueryFingerprintUtils() {
+ }
+
+ @Nullable
+ public static QueryFingerprint generateFingerprint(SqlNodeAndOptions sqlNodeAndOptions) throws Exception {
+ if (sqlNodeAndOptions == null) {
+ return null;
+ }
+
+ SqlNode sqlNode = sqlNodeAndOptions.getSqlNode();
+ if (sqlNode == null) {
+ return null;
+ }
+
+ QueryFingerprintVisitor visitor = new QueryFingerprintVisitor();
+ SqlNode queryFingerprintNode = sqlNode.accept(visitor);
+
+ if (queryFingerprintNode == null) {
+ return null;
+ }
+
+ String fingerprint = queryFingerprintNode.toSqlString(c -> c.withDialect(AnsiSqlDialect.DEFAULT)).getSql();
+
+ if (fingerprint == null) {
+ return null;
+ }
+
+ // Normalize whitespace: replace newlines with spaces, collapse multiple spaces into one, and trim
+ fingerprint = fingerprint.replace("\n", " ").replaceAll("\\s+", " ").trim();
+
+ return new QueryFingerprint(hashString(fingerprint), fingerprint);
+ }
+
+ private static String hashString(String input) {
+ return Hashing.farmHashFingerprint64()
+ .hashString(input, StandardCharsets.UTF_8)
+ .toString();
+ }
Review Comment:
updated summary.
--
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-unsubscribe@pinot.apache.org
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
Re: [PR] Add Query Fingerprint Feature [pinot]
Posted by "MeihanLi (via GitHub)" <gi...@apache.org>.
MeihanLi commented on code in PR #17177:
URL: https://github.com/apache/pinot/pull/17177#discussion_r2535566663
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/QueryFingerprintVisitor.java:
##########
@@ -0,0 +1,214 @@
+/**
+ * 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.utils.request;
+
+import java.util.ArrayList;
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlJoin;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlOrderBy;
+import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.fun.SqlCase;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.util.SqlShuttle;
+
+
+/**
+ * QueryFingerprintVisitor traverses the Calcite SqlNode AST and produces a normalized query fingerprint.
+ * Implementation is based on Calcite 1.40.0 version. It may change in future versions of Calcite.
+ *
+ * <p>
+ * <ul>
+ * <li>All data literals are replaced with dynamic parameters (?)</li>
+ * <li>Symbolic keywords (DISTINCT, ASC, DESC, etc.) and NULL literals are preserved.</li>
+ * <li>Hints are preserved.</li>
+ * <li>EXPLAIN PLAN FOR is NOT preserved.</li>
+ * </ul>
+ * </p>
+ */
+public class QueryFingerprintVisitor extends SqlShuttle {
+ // SqlSelect operand index for hints, see {@link org.apache.calcite.sql.SqlSelect}.
+ private static final int HINTS_OPERAND_INDEX = 11;
Review Comment:
Thanks for the comment, addressed.
--
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-unsubscribe@pinot.apache.org
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