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

Add SPI for delegating row expression optimizer #24144

Merged
merged 3 commits into from
Jan 29, 2025

Conversation

tdcmeehan
Copy link
Contributor

@tdcmeehan tdcmeehan commented Nov 25, 2024

Description

As part of RFC-0006, we need to support out of process expression evaluation. Add support for pluggable expression optimization and planner support to utilize the new SPI.

Motivation and Context

RFC-0006. See #24126 for larger changes that include the Presto sidecar as described in the RFC.

Impact

No impact by default as the old in-memory evaluation is the default.

Test Plan

Tests have been added.

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • 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

== RELEASE NOTES ==

SPI Changes
* Add ``CoordinatorPlugin#getExpressionOptimizerFactories`` to customize expression evaluation in the Presto coordinator. :pr:`24144`

Sorry, something went wrong.

@tdcmeehan tdcmeehan requested a review from presto-oss November 25, 2024 18:56
@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Nov 25, 2024
@prestodb-ci prestodb-ci requested review from a team, sh-shamsan and pdabre12 and removed request for a team November 25, 2024 18:56
Copy link
Contributor

@pdabre12 pdabre12 left a comment

Choose a reason for hiding this comment

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

Thanks @tdcmeehan.
Just some initial comments

@tdcmeehan tdcmeehan force-pushed the expr-spi-init branch 12 times, most recently from 17fd8e1 to 563611e Compare December 2, 2024 17:34
Copy link

github-actions bot commented Dec 2, 2024

Codenotify: Notifying subscribers in CODENOTIFY files for diff 3f4e4c3...4f56709.

No notifications.

@tdcmeehan tdcmeehan force-pushed the expr-spi-init branch 3 times, most recently from 3cbc135 to 6e124d2 Compare January 9, 2025 20:53
@tdcmeehan tdcmeehan force-pushed the expr-spi-init branch 6 times, most recently from 9d80d3e to 2cf08d2 Compare January 17, 2025 21:37
@tdcmeehan
Copy link
Contributor Author

tdcmeehan commented Jan 21, 2025

Updates:

  1. Removed DelegatingExpressionOptimizer and all related tests.
  2. Added the ability to configure multiple expression optimizers and switch them based on a session property. Should be useful for fuzzer testing.

Thanks for the comments, please take another @ZacBlanco @rschlussel @aaneja @pdabre12

@tdcmeehan tdcmeehan force-pushed the expr-spi-init branch 2 times, most recently from 616bb09 to 226a3c3 Compare January 22, 2025 21:23
Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

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

Two comments, otherwise lgtm


public ExpressionOptimizer getExpressionOptimizer(ConnectorSession connectorSession)
{
checkArgument(connectorSession instanceof FullConnectorSession, "connectorSession is not an instance of FullConnectorSession");
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't like assuming that a session is a connectorSession. it breaks the abstractions. In this case all we care about is the session property. can we instead add a getSystemProperty() method to ConnectorSession?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I agree this is not ideal, I'm of the opinion that it's worse to expose getSystemProperty to the SPI, because this allows all plugins to access system properties that were previously only known to the engine or runtime. While not ideal, I think this approach is a bit less worse because it's within presto-main, and we have examples of this being done for system connector and metadata tables provided to the JDBC driver. I do think that we should probably think of a better abstraction for how we provide session properties to plugins which are not connectors, and I've added a TODO and will open a Github issue to track it. Let me know if you think this is reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair criticism and suggestion to punt on resolving this. i do feel like the problem i'm talking about isn't about plugins that aren't connectors, it's easy enough to implement passing them ConnectorSessions. The thing that makes this require a ConnectorSession vs. a Session is that it gets called from connector optimizers. and we had similar issue with stats stuff a while back. The plugin separation break down when connectors call into stuff from presto-main.

Copy link
Contributor

@pdabre12 pdabre12 left a comment

Choose a reason for hiding this comment

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

Just two nits, otherwise LGTM.

The runtime should consolidate to the `ExpressionOptimizerProvider` factory
so that it can be customized without significant refactoring.
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

+1 for presto-native-execution changes.

@tdcmeehan tdcmeehan merged commit c229714 into prestodb:master Jan 29, 2025
58 of 59 checks passed
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.

None yet

7 participants