This is an automated email from the ASF dual-hosted git repository. jhyde pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/calcite.git
commit a250ab0de827f221eb0bec780f5454f33cd7e782 Author: Julian Hyde <[email protected]> AuthorDate: Wed Sep 22 23:40:00 2021 -0700 [CALCITE-4795] In class SqlBasicCall, make the "operands" field private (part 1) In class SqlBasicCall, the operands field is currently a public array. This gives too much freedom for clients to do crazy things, and creates unexpected behavior because the array is not copied when a SqlBasicCall is cloned. We would like to make the first private, but that would be a breaking change, so we must do it over two releases. This is part 1: deprecate the "operands" field; after 1.28, part 2 will make the "operands" field private, and also change it from an array to an immutable List. You will still be able to call SqlCall.setOperand, but the whole new List will be created. --- .../apache/calcite/rel/rel2sql/SqlImplementor.java | 2 +- .../java/org/apache/calcite/sql/SqlBasicCall.java | 63 +++++++++++++++++++--- .../calcite/sql/SqlNullTreatmentOperator.java | 2 +- .../java/org/apache/calcite/sql/SqlOperator.java | 2 +- .../calcite/sql/validate/SqlValidatorImpl.java | 2 +- .../apache/calcite/sql2rel/SqlToRelConverter.java | 11 ++-- .../calcite/sql2rel/StandardConvertletTable.java | 3 +- 7 files changed, 67 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java b/core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java index 0751ea8..ce6d1fb 100644 --- a/core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java +++ b/core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java @@ -1821,7 +1821,7 @@ public abstract class SqlImplementor { if (selectList.get(aggregatesArg) instanceof SqlBasicCall) { final SqlBasicCall call = (SqlBasicCall) selectList.get(aggregatesArg); - for (SqlNode operand : call.getOperands()) { + for (SqlNode operand : call.getOperandList()) { if (operand != null && operandPredicate.test(operand)) { return true; } diff --git a/core/src/main/java/org/apache/calcite/sql/SqlBasicCall.java b/core/src/main/java/org/apache/calcite/sql/SqlBasicCall.java index 586917e..246748f 100755 --- a/core/src/main/java/org/apache/calcite/sql/SqlBasicCall.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlBasicCall.java @@ -17,10 +17,11 @@ package org.apache.calcite.sql; import org.apache.calcite.sql.parser.SqlParserPos; -import org.apache.calcite.util.UnmodifiableArrayList; +import org.apache.calcite.util.ImmutableNullableList; import org.checkerframework.checker.nullness.qual.Nullable; +import java.util.Arrays; import java.util.List; import java.util.Objects; @@ -31,7 +32,16 @@ import static org.apache.calcite.linq4j.Nullness.castNonNull; */ public class SqlBasicCall extends SqlCall { private SqlOperator operator; + + /** Array of operands. + * + * @deprecated Use the methods {@link #getOperandList()} and + * {@link #setOperand(int, SqlNode)}. To be removed before 1.29. + */ + @Deprecated // to be removed before 1.29 public final @Nullable SqlNode[] operands; + + private final List<@Nullable SqlNode> operandList; private final @Nullable SqlLiteral functionQuantifier; private final boolean expanded; @@ -39,18 +49,45 @@ public class SqlBasicCall extends SqlCall { SqlOperator operator, @Nullable SqlNode[] operands, SqlParserPos pos) { - this(operator, operands, pos, false, null); + this(operator, operands, pos, null, false); } + /** Creates a SqlBasicCall. + * + * @deprecated Use + * {@link #SqlBasicCall(SqlOperator, SqlNode[], SqlParserPos, SqlLiteral)} + * followed by {@link #withExpanded(boolean)}. To be removed before 1.29. + */ + @Deprecated // to be removed before 1.29 public SqlBasicCall( SqlOperator operator, @Nullable SqlNode[] operands, SqlParserPos pos, boolean expanded, @Nullable SqlLiteral functionQualifier) { + this(operator, operands, pos, functionQualifier, expanded); + } + + /** Creates an unexpanded SqlBasicCall. */ + public SqlBasicCall( + SqlOperator operator, + @Nullable SqlNode[] operands, + SqlParserPos pos, + @Nullable SqlLiteral functionQualifier) { + this(operator, operands, pos, functionQualifier, false); + } + + /** Private constructor. */ + private SqlBasicCall( + SqlOperator operator, + @Nullable SqlNode[] operands, + SqlParserPos pos, + @Nullable SqlLiteral functionQualifier, + boolean expanded) { super(pos); this.operator = Objects.requireNonNull(operator, "operator"); this.operands = operands; + this.operandList = Arrays.asList(operands); this.expanded = expanded; this.functionQuantifier = functionQualifier; } @@ -63,8 +100,14 @@ public class SqlBasicCall extends SqlCall { return expanded; } + public SqlCall withExpanded(boolean expanded) { + return expanded == this.expanded ? this + : new SqlBasicCall(operator, operandList.toArray(new SqlNode[0]), pos, + functionQuantifier, expanded); + } + @Override public void setOperand(int i, @Nullable SqlNode operand) { - operands[i] = operand; + operandList.set(i, operand); } public void setOperator(SqlOperator operator) { @@ -75,22 +118,28 @@ public class SqlBasicCall extends SqlCall { return operator; } + /** Returns the array of operands. + * + * @deprecated Use the methods {@link #getOperandList()} and + * {@link #setOperand(int, SqlNode)}. To be removed before 1.29. + */ + @Deprecated // to be removed before 1.29 public @Nullable SqlNode[] getOperands() { return operands; } @SuppressWarnings("nullness") @Override public List<SqlNode> getOperandList() { - return UnmodifiableArrayList.of(operands); // not immutable, but quick + return ImmutableNullableList.copyOf(operandList); } @SuppressWarnings("unchecked") @Override public <S extends SqlNode> S operand(int i) { - return (S) castNonNull(operands[i]); + return (S) castNonNull(operandList.get(i)); } @Override public int operandCount() { - return operands.length; + return operandList.size(); } @Override public @Nullable SqlLiteral getFunctionQuantifier() { @@ -98,7 +147,7 @@ public class SqlBasicCall extends SqlCall { } @Override public SqlNode clone(SqlParserPos pos) { - return getOperator().createCall(getFunctionQuantifier(), pos, operands); + return getOperator().createCall(getFunctionQuantifier(), pos, operandList); } } diff --git a/core/src/main/java/org/apache/calcite/sql/SqlNullTreatmentOperator.java b/core/src/main/java/org/apache/calcite/sql/SqlNullTreatmentOperator.java index 4b055cb..d70046f 100644 --- a/core/src/main/java/org/apache/calcite/sql/SqlNullTreatmentOperator.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlNullTreatmentOperator.java @@ -47,7 +47,7 @@ public class SqlNullTreatmentOperator extends SqlSpecialOperator { @Override public SqlCall createCall(@Nullable SqlLiteral functionQualifier, SqlParserPos pos, @Nullable SqlNode... operands) { // As super.createCall, but don't union the positions - return new SqlBasicCall(this, operands, pos, false, functionQualifier); + return new SqlBasicCall(this, operands, pos, functionQualifier); } @Override public void unparse(SqlWriter writer, SqlCall call, int leftPrec, diff --git a/core/src/main/java/org/apache/calcite/sql/SqlOperator.java b/core/src/main/java/org/apache/calcite/sql/SqlOperator.java index ece841a..4d89a12 100644 --- a/core/src/main/java/org/apache/calcite/sql/SqlOperator.java +++ b/core/src/main/java/org/apache/calcite/sql/SqlOperator.java @@ -274,7 +274,7 @@ public abstract class SqlOperator { SqlParserPos pos, @Nullable SqlNode... operands) { pos = pos.plusAll(operands); - return new SqlBasicCall(this, operands, pos, false, functionQualifier); + return new SqlBasicCall(this, operands, pos, functionQualifier); } /** Not supported. Choose between diff --git a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java index 28c0755..0f607d9 100644 --- a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java +++ b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java @@ -1822,7 +1822,7 @@ public class SqlValidatorImpl implements SqlValidatorWithHints { // call to this function, so we can use the regular // operator validation. return new SqlBasicCall(operator, SqlNode.EMPTY_ARRAY, - id.getParserPosition(), true, null); + id.getParserPosition(), null).withExpanded(true); } } } diff --git a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java index b872137..88a326d 100644 --- a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java +++ b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java @@ -1683,7 +1683,7 @@ public class SqlToRelConverter { if (isRowConstructor(node)) { call = (SqlBasicCall) node; ImmutableList.Builder<RexLiteral> tuple = ImmutableList.builder(); - for (Ord<@Nullable SqlNode> operand : Ord.zip(call.operands)) { + for (Ord<SqlNode> operand : Ord.zip(call.getOperandList())) { RexLiteral rexLiteral = convertLiteralInValuesList( operand.e, @@ -2115,7 +2115,6 @@ public class SqlToRelConverter { } final SqlCall call; - final @Nullable SqlNode[] operands; switch (from.getKind()) { case AS: call = (SqlCall) from; @@ -2147,20 +2146,20 @@ public class SqlToRelConverter { return; case TABLESAMPLE: - operands = ((SqlBasicCall) from).getOperands(); + final List<SqlNode> operands = ((SqlCall) from).getOperandList(); SqlSampleSpec sampleSpec = SqlLiteral.sampleValue( - requireNonNull(operands[1], () -> "operand[1] of " + from)); + requireNonNull(operands.get(1), () -> "operand[1] of " + from)); if (sampleSpec instanceof SqlSampleSpec.SqlSubstitutionSampleSpec) { String sampleName = ((SqlSampleSpec.SqlSubstitutionSampleSpec) sampleSpec) .getName(); datasetStack.push(sampleName); - convertFrom(bb, operands[0]); + convertFrom(bb, operands.get(0)); datasetStack.pop(); } else if (sampleSpec instanceof SqlSampleSpec.SqlTableSampleSpec) { SqlSampleSpec.SqlTableSampleSpec tableSampleSpec = (SqlSampleSpec.SqlTableSampleSpec) sampleSpec; - convertFrom(bb, operands[0]); + convertFrom(bb, operands.get(0)); RelOptSamplingParameters params = new RelOptSamplingParameters( tableSampleSpec.isBernoulli(), diff --git a/core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java b/core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java index dc8d53a..9e0bc9e 100644 --- a/core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java +++ b/core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java @@ -1152,7 +1152,8 @@ public class StandardConvertletTable extends ReflectiveConvertletTable { private RexNode toRex(SqlRexContext cx, SqlBasicCall call, SqlFunction f) { final SqlCall call2 = - new SqlBasicCall(f, call.operands, call.getParserPosition()); + new SqlBasicCall(f, call.getOperandList().toArray(new SqlNode[0]), + call.getParserPosition()); final SqlRexConvertlet convertlet = requireNonNull(get(call2)); return convertlet.convertCall(cx, call2); }
