SqlBasicCall.clone shallow copy

[Wen Zhang <zh...@cs.berkeley.edu>]

Hello,

I'm trying to make shallow copies of `SqlNode` objects by calling the 
`clone(SqlParserPos pos) 
<https://calcite.apache.org/javadocAggregate/org/apache/calcite/sql/SqlNode.html#clone(org.apache.calcite.sql.parser.SqlParserPos)>` 
method. However, the `SqlBasicCall` class overrides this method with an 
implementation 
<https://github.com/apache/calcite/blob/4b1e9ed1a513eae0c873a660b755bb4f27b39da9/core/src/main/java/org/apache/calcite/sql/SqlBasicCall.java#L100> 
that might return a new `SqlCall` that shares the operand array with the 
original. This is due to it calling `SqlOperator.createCall(SqlLiteral 
functionQualifier, SqlParserPos pos, SqlNode... operands) 
<https://calcite.apache.org/javadocAggregate/org/apache/calcite/sql/SqlOperator.html#createCall(org.apache.calcite.sql.SqlLiteral,org.apache.calcite.sql.parser.SqlParserPos,org.apache.calcite.sql.SqlNode...)>`, 
whose default implementation 
<https://github.com/apache/calcite/blob/4b1e9ed1a513eae0c873a660b755bb4f27b39da9/core/src/main/java/org/apache/calcite/sql/SqlOperator.java#L272> 
creates a new `SqlBasicCall` object without copying `operands`.

Is my understanding correct?

Thank you!

Best regards,
Wen

Re: SqlBasicCall.clone shallow copy

[Julian Hyde <jh...@gmail.com>]

That looks like a bug. Cloning a SqlCall and then calling setOperand on it is valid, and should not modify the original SqlCall.

We tend to avoid calling ’set’ methods on SqlNodes - we treat them as immutable except that the validator will replace an argument with an equivalent argument (e.g. replacing an identifier with a fully-qualified identifier). That’s an explanation of why we don’t tend to hit this problem - I’m not claiming it’s not a bug.

Can you log a JIRA case please?

Separately, it’s crazy that the “operands" field [1] of SqlBasicCall is public. We should deprecate the field for one point release (mirroring it into a private mutable list field) and then remove it. Later we can make the private list field immutable (copy on write) or something.

Julian

[1] https://github.com/apache/calcite/blob/4bc916619fd286b2c0cc4d5c653c96a68801d74e/core/src/main/java/org/apache/calcite/sql/SqlBasicCall.java#L34 <https://github.com/apache/calcite/blob/4bc916619fd286b2c0cc4d5c653c96a68801d74e/core/src/main/java/org/apache/calcite/sql/SqlBasicCall.java#L34> 




> On Sep 22, 2021, at 3:36 PM, Wen Zhang <zh...@cs.berkeley.edu> wrote:
> 
> Hello,
> 
> I'm trying to make shallow copies of `SqlNode` objects by calling the `clone(SqlParserPos pos) <https://calcite.apache.org/javadocAggregate/org/apache/calcite/sql/SqlNode.html#clone(org.apache.calcite.sql.parser.SqlParserPos)>` method. However, the `SqlBasicCall` class overrides this method with an implementation <https://github.com/apache/calcite/blob/4b1e9ed1a513eae0c873a660b755bb4f27b39da9/core/src/main/java/org/apache/calcite/sql/SqlBasicCall.java#L100> that might return a new `SqlCall` that shares the operand array with the original. This is due to it calling `SqlOperator.createCall(SqlLiteral functionQualifier, SqlParserPos pos, SqlNode... operands) <https://calcite.apache.org/javadocAggregate/org/apache/calcite/sql/SqlOperator.html#createCall(org.apache.calcite.sql.SqlLiteral,org.apache.calcite.sql.parser.SqlParserPos,org.apache.calcite.sql.SqlNode...)>`, whose default implementation <https://github.com/apache/calcite/blob/4b1e9ed1a513eae0c873a660b755bb4f27b39da9/core/src/main/java/org/apache/calcite/sql/SqlOperator.java#L272> creates a new `SqlBasicCall` object without copying `operands`.
> 
> Is my understanding correct?
> 
> Thank you!
> 
> Best regards,
> Wen
>