Skip to content

[CALCITE-5949] RexExecutable should return unchanged original expressions when it fails#3390

Closed
arkanovicz wants to merge 2 commits intoapache:mainfrom
arkanovicz:reduction-errors
Closed

[CALCITE-5949] RexExecutable should return unchanged original expressions when it fails#3390
arkanovicz wants to merge 2 commits intoapache:mainfrom
arkanovicz:reduction-errors

Conversation

@arkanovicz
Copy link
Contributor

@arkanovicz arkanovicz commented Aug 22, 2023 •
edited
Loading

While reducing, when encountering an invalid expression in the list of constant expressions, RexExecutor is meant to return all initial expressions unchanged.

It fails to do so, because already handled correct expressions have already been added to the returned list, which can be greater than the input list.

For instance, when given the list { LN(2), LN(-2) }, the RexExecutor will output a list of length 3.

This PR corrects this problem, and adds a corresponding test case.

@arkanovicz arkanovicz changed the title CALCITE-5949 - Correct handling of invalid constant expressions in reduction [CALCITE-5949] Correct handling of invalid constant expressions in reduction Aug 22, 2023
@arkanovicz arkanovicz force-pushed the reduction-errors branch 2 times, most recently from a9c2c32 to b2cd019 Compare August 22, 2023 14:15
@rubenada
Copy link
Contributor

rubenada commented Aug 23, 2023

Patch looks good.
Please note that there are some comments in Jira (that I subscribe) that request to change the Jira title (and hence the PR title and the commit message). I'd propose "RexExecutable returns incorrect result if invalid expression is processed after valid ones" or something a long the lines.

rubenada
rubenada reviewed Aug 23, 2023
View reviewed changes
@rubenada rubenada self-assigned this Aug 23, 2023
@arkanovicz arkanovicz changed the title [CALCITE-5949] Correct handling of invalid constant expressions in reduction [CALCITE-5949] RexExecutable should properly handle invalid constant expressions in reduction Aug 23, 2023
@rubenada
Copy link
Contributor

rubenada commented Sep 7, 2023

@arkanovicz could you please rebase with latest main and squash commits?

@arkanovicz arkanovicz changed the title [CALCITE-5949] RexExecutable should properly handle invalid constant expressions in reduction [CALCITE-5949] RexExecutable should return unchanged original expressions when it fails Sep 10, 2023
@arkanovicz arkanovicz force-pushed the reduction-errors branch 2 times, most recently from b32a971 to bd304e7 Compare September 10, 2023 12:16
@arkanovicz
Copy link
Contributor Author

arkanovicz commented Sep 10, 2023

@arkanovicz could you please rebase with latest main and squash commits?

@rubenada Done.

rubenada
rubenada reviewed Sep 11, 2023
View reviewed changes
core/src/test/java/org/apache/calcite/rex/RexExecutorTest.java

/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-5949">[CALCITE-5949]
* RexExecutable should properly handle invalid constant expressions in reduction</a>.
Copy link
Contributor

@rubenada rubenada Sep 11, 2023

Choose a reason for hiding this comment

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

@arkanovicz could you please align this comment with the latest title / commit message ("RexExecutable should return unchanged original expressions when it fails")?
Then please squash commits into a single one, and I'll proceed with the merge

Copy link
Contributor

@rubenada rubenada Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@arkanovicz kind reminder about this minor (old) remark regarding the ticket title on this javadoc.
Feel free to squash commits into a single one after you take care of it; and I'll merge the PR afterwards.

Copy link
Contributor

@rubenada rubenada Oct 9, 2023

Choose a reason for hiding this comment

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

@arkanovicz we are approaching the preparation for the next release, could you please carry out these last details ⬆️ in order to merge this PR? thanks

@rubenada rubenada added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 11, 2023
rubenada
rubenada reviewed Oct 2, 2023
View reviewed changes
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

83.3% 83.3% Coverage
0.0% 0.0% Duplication

@libenchao
Copy link
Member

libenchao commented Oct 24, 2023

@arkanovicz Are you available to move this forward and get it in 1.36.0? It seems this PR is in a very good shape already.

@rubenada
Copy link
Contributor

rubenada commented Oct 25, 2023

@libenchao , considering that we are approaching the RC deadline (it would be sad to not include this on it); in case @arkanovicz cannot respond in 24h, I'll create a separate PR (tagging him as co-author) to finalize this fix.

@rubenada rubenada mentioned this pull request Oct 26, 2023
Merged
@rubenada
Copy link
Contributor

rubenada commented Oct 27, 2023

Closing this in favor of #3486

@rubenada rubenada closed this Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rubenada rubenada rubenada left review comments

@JiajunBernoulli JiajunBernoulli JiajunBernoulli left review comments

Assignees

@rubenada rubenada

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@arkanovicz @rubenada @libenchao @JiajunBernoulli