letian-jiang commented on code in PR #3086: URL: https://github.com/apache/drill/pull/3086#discussion_r4390300728
########## exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/PlanCacheParameterizer.java: ########## @@ -0,0 +1,208 @@ +/* + * 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.drill.exec.planner.sql; + +import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Locale; +import java.util.Objects; + +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.sql.SqlCall; +import org.apache.calcite.sql.SqlJoin; +import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.SqlLiteral; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.SqlOrderBy; +import org.apache.calcite.sql.SqlNodeList; +import org.apache.calcite.sql.SqlSelect; +import org.apache.calcite.sql.type.SqlTypeName; +import org.apache.calcite.sql.util.SqlShuttle; +import org.apache.calcite.util.NlsString; +import org.apache.drill.exec.planner.sql.conversion.SqlBoundDynamicParam; + +/** Extracts SQL literals into typed slots independently of cache eligibility. */ +final class PlanCacheParameterizer extends SqlShuttle { + private final List<SqlLiteral> literals = new ArrayList<>(); + private final List<String> types = new ArrayList<>(); + private final RelDataTypeFactory typeFactory; + private boolean preserveProjection; + + private PlanCacheParameterizer(RelDataTypeFactory typeFactory) { + this.typeFactory = Objects.requireNonNull(typeFactory, "typeFactory"); + } + + static Candidate parameterize(SqlNode parsed, RelDataTypeFactory typeFactory) { + Objects.requireNonNull(parsed, "parsed"); + PlanCacheParameterizer visitor = new PlanCacheParameterizer(typeFactory); + SqlNode parameterized = parsed.accept(visitor); + if (visitor.literals.isEmpty()) { + parameterized = parsed; + } + return new Candidate(parameterized, + Collections.unmodifiableList(visitor.literals), + parameterized.toString() + visitor.types); + } + + @Override + public SqlNode visit(SqlCall call) { + int configurationOperand = configurationOperand(call); + if (configurationOperand >= 0 && configurationOperand < call.operandCount()) { + // These operands become function names, encodings or resolved plugin + // references during conversion. They are not bindable expressions in + // the physical plan, so keep their entire subtree in the template key. + SqlCall parameterized = (SqlCall) call.clone(call.getParserPosition()); + for (int i = 0; i < call.operandCount(); i++) { + if (i != configurationOperand) { + parameterized.setOperand(i, visitNullable(call.operand(i))); + } + } + return parameterized; + } + if ("COALESCE".equalsIgnoreCase(call.getOperator().getName()) + && call.getOperandList().stream() + .anyMatch(operand -> operand instanceof SqlLiteral + && ((SqlLiteral) operand).getTypeName() == SqlTypeName.NULL)) { + // Calcite cannot infer a dynamic parameter's type alongside an untyped + // NULL. Keep this call in the key; explicit casts still allow slots. + return call; + } + if (call instanceof SqlJoin) { + SqlJoin join = (SqlJoin) call; + return new SqlJoin(join.getParserPosition(), visitNullable(join.getLeft()), + join.isNaturalNode(), join.getJoinTypeNode(), visitNullable(join.getRight()), + join.getConditionTypeNode(), visitNullable(join.getCondition())); + } + if (call.getKind() == SqlKind.ITEM && call.operand(1) instanceof SqlLiteral) { + // A map/array key identifies a field in the input schema. Changing it can + // change the scan projection and the HBase column used by pushdown. + return call.getOperator().createCall(call.getParserPosition(), + visitNullable(call.operand(0)), call.operand(1)); + } + // These literals control rows, ordinals, window frames or operator + // configuration. Keep them verbatim in the template key. + if (call.getKind() == SqlKind.VALUES || call.getKind() == SqlKind.ORDER_BY + || call.getKind() == SqlKind.OVER) { + if (call instanceof SqlOrderBy) { + SqlOrderBy orderBy = (SqlOrderBy) call; + boolean previous = preserveProjection; + preserveProjection = true; + SqlNode query; + try { + query = orderBy.query.accept(this); + } finally { + preserveProjection = previous; + } + return new SqlOrderBy(call.getParserPosition(), query, orderBy.orderList, + orderBy.offset, orderBy.fetch); + } + return call; + } + if (call instanceof SqlSelect) { + SqlSelect select = (SqlSelect) call; + // Preserve matching expressions across SELECT, GROUP BY, HAVING and + // QUALIFY. Parameterizing only some occurrences breaks validation or rewrites. + boolean structuralProjection = preserveProjection Review Comment: I now preserve literals in SELECT, HAVING and QUALIFY unconditionally. For example, `SELECT x + 1` keeps the `1` in the template key. I expect most of the benefit to come from rebinding WHERE predicates, so this conservative restriction seems reasonable. Added tests for QUALIFY without an outer ORDER BY and inline windows. ########## contrib/format-iceberg/src/main/java/org/apache/drill/exec/store/iceberg/plan/DrillExprToIcebergTranslator.java: ########## @@ -103,7 +145,7 @@ public Expression visitFunctionCall(FunctionCall call, Void value) throws Runtim } case FunctionNames.GE: { LogicalExpression nameRef = call.args().get(0); - Expression expression = call.args().get(0).accept(this, null); + Expression expression = call.args().get(1).accept(this, null); Review Comment: Added translator and cache-off query tests for `>=`, AND/OR and temporal values. Also fixed NULL handling and retained Drill filtering when `ignoreResiduals=true`. ########## contrib/format-iceberg/src/main/java/org/apache/drill/exec/store/iceberg/plan/IcebergPluginImplementor.java: ########## @@ -67,6 +70,10 @@ public void implement(PluginFilterRel filter) throws IOException { new DrillParseContext(PrelUtil.getPlannerSettings(filter.getCluster().getPlanner())), filter.getInput(), condition); + if (groupScan.getCondition() != null) { Review Comment: Added an implementor regression test for two stacked filters, such as `id >= 2 AND id < 4`. It checks values inside and outside the range to verify that both bounds are retained. -- 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]
