Skip to content

[CALCITE-6706] Checked arithmetic does not take effect in subqueries#4066

Merged
mihaibudiu merged 1 commit intoapache:mainfrom
mihaibudiu:issue6706
Nov 28, 2024
Merged

[CALCITE-6706] Checked arithmetic does not take effect in subqueries#4066
mihaibudiu merged 1 commit intoapache:mainfrom
mihaibudiu:issue6706

Conversation

@mihaibudiu
Copy link
Contributor

No description provided.

@mihaibudiu mihaibudiu requested a review from NobiGo November 27, 2024 02:47
@mihaibudiu
Copy link
Contributor Author

@NobiGo, I invited you to review since you commented on the issue. I have added the tests you suggested.


@Override public RexNode visitSubQuery(RexSubQuery subQuery) {
RelNode result = subQuery.rel.accept(ConvertToChecked.this);
return subQuery.clone(result);
Copy link
Member

@caicancai caicancai Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is clone necessary? I understand that clone should be avoided if it can be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the Java Object.clone() method, that one has no arguments.

You can see the JavaDoc for the clone methods for RexCall in RexCall.clone().

  /**
   * Creates a new call to the same operator with different operands.
   *
   * @param type     Return type
   * @param operands Operands to call
   * @return New call
   */
  public RexCall clone(RelDataType type, List<RexNode> operands) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If result==subQuery.rel, do we still need to clone it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, didn't see this until too late, if this is redundant I will fix it in a subsequent PR.
Have to find out whether clone makes the check or not.

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
@sonarqubecloud
Copy link

@mihaibudiu mihaibudiu merged commit fa0d559 into apache:main Nov 28, 2024
@mihaibudiu mihaibudiu deleted the issue6706 branch November 28, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants