Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ExpressionOptimizer #24287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tdcmeehan
Copy link
Contributor

@tdcmeehan tdcmeehan commented Dec 19, 2024

Description

Refactor ExpressionOptimizer to consistently return RowExpression. Before, ExpressionOptimizer would return RowExpression only for the method which did not take in a variable resolver function. I cannot discern a good reason for this. By refactoring this interface and making them both return RowExpression, and documenting that the two methods are equivalent when an identity function is used for the variable resolver, it becomes possible to share code that uses both methods.

Motivation and Context

This is to help address PR review feedback here: #24144 (comment)

Impact

Code refactoring. Implementations of ExpressionOptimizer in the SPI will need to update their plugins to accommodate the changed API.

Test Plan

Unit tests

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

SPI Changes
* Improve ExpressionOptimizer#optimize method with a variable resolver to return ``RowExpression``. :pr:`24287`

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Dec 19, 2024
@prestodb-ci prestodb-ci requested review from a team, czentgr and aditi-pandit and removed request for a team December 19, 2024 17:30
@ZacBlanco
Copy link
Contributor

Don't forget to update the release note with the correct PR #

Copy link
Member

@hantangwangd hantangwangd left a comment

Choose a reason for hiding this comment

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

Thanks for this refactor, LGTM!

{
RowExpressionInterpreter interpreter = new RowExpressionInterpreter(expression, metadata.getFunctionAndTypeManager(), session, level);
return interpreter.optimize(variableResolver::apply);
return toRowExpression(expression.getSourceLocation(), interpreter.optimize(variableResolver::apply), expression.getType());
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to change RowExpressionInterpreter to return a RowExpression instead of an object (seems like basically it requires wrapping any "values" in constant expressions, and then changing anything that calls it to expect that to happen). Returning an Object that could be anything and leaving it to the caller be able to handle it seems like a bad pattern to begin with.

Copy link
Contributor

Choose a reason for hiding this comment

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

but this change is still an improvement, so i'm fine with merging this first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants