Skip to content

[CALCITE-5861] Optimization rules do not constant-fold expressions in window bounds#3328

Merged
asolimando merged 1 commit intoapache:mainfrom
mihaibudiu:issue5861
Sep 1, 2023
Merged

[CALCITE-5861] Optimization rules do not constant-fold expressions in window bounds#3328
asolimando merged 1 commit intoapache:mainfrom
mihaibudiu:issue5861

Conversation

@mihaibudiu
Copy link
Copy Markdown
Contributor

With this change the PROJECT_REDUCE_EXPRESSIONS rule will also optimize constants that appear in window bound expressions. This makes it possible to compile programs that use (constant) expressions in window bounds.

However, I suspect that the rule PROJECT_TO_LOGICAL_PROJECT_AND_WINDOW is still buggy, since it will move window bound expressions into separate projections, as described in the JIRA case. Unfortunately I could not figure out how to fix that rule - the code is very generic and treats all expressions in the same way, regardless where they appear in the RexNode. Perhaps I should file another JIRA issue about this?

@mihaibudiu
Copy link
Copy Markdown
Contributor Author

I would very much appreciate a review for this tiny PR.

@asolimando
Copy link
Copy Markdown
Member

However, I suspect that the rule PROJECT_TO_LOGICAL_PROJECT_AND_WINDOW is still buggy, since it will move window bound expressions into separate projections, as described in the JIRA case. Unfortunately I could not figure out how to fix that rule - the code is very generic and treats all expressions in the same way, regardless where they appear in the RexNode. Perhaps I should file another JIRA issue about this?

I agree on separating the two problems and filing a separate Jira about moving the window bound expressions in a projection.

@asolimando
Copy link
Copy Markdown
Member

@mihaibudiu, if you file a different ticket for the projection part, I guess the test you added will look different and involve only ReduceExpressionsRule, right?

In addition, in general, we tend to phrase the Jira ticket titles a bit differently though, something like "ReduceExpressionsRule rules should constant-fold expressions in window bounds", WDYT?

@mihaibudiu
Copy link
Copy Markdown
Contributor Author

In the JIRA ticket discussion @julianhyde said this:

The SQL spec doesn't 'require' that bounds are constant. It says that a conforming implementation shall allow bounds that are constant. It doesn't say that a conforming implementation shall not allow bounds that are not constant.
If I recall correctly, Oracle allows expressions. It's a nice feature. We should continue to allow expressions, even if Postgres doesn't.

So I think based on this interpretation a second issue will not be necessary. I will rename the issue as you suggested.

…ssions in window bounds

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

sonarqubecloud bot commented Aug 1, 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 0 Code Smells

87.5% 87.5% Coverage
0.0% 0.0% Duplication

Copy link
Copy Markdown
Contributor

@NobiGo NobiGo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@asolimando asolimando left a comment

Choose a reason for hiding this comment

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

LGTM too

@mihaibudiu
Copy link
Copy Markdown
Contributor Author

Maybe someone can merge this?

@asolimando asolimando merged commit cbf857e into apache:main Sep 1, 2023
@mihaibudiu mihaibudiu deleted the issue5861 branch September 1, 2023 17:58
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.

4 participants