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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
