Add SPI for delegating row expression optimizer#24144
Conversation
pdabre12
left a comment
There was a problem hiding this comment.
Thanks @tdcmeehan.
Just some initial comments
17fd8e1 to
563611e
Compare
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 3f4e4c3...4f56709. No notifications. |
563611e to
948cf13
Compare
948cf13 to
d25b1b6
Compare
9d80d3e to
2cf08d2
Compare
|
Updates:
Thanks for the comments, please take another @ZacBlanco @rschlussel @aaneja @pdabre12 |
616bb09 to
226a3c3
Compare
ZacBlanco
left a comment
There was a problem hiding this comment.
Two comments, otherwise lgtm
226a3c3 to
92e8cd3
Compare
|
|
||
| public ExpressionOptimizer getExpressionOptimizer(ConnectorSession connectorSession) | ||
| { | ||
| checkArgument(connectorSession instanceof FullConnectorSession, "connectorSession is not an instance of FullConnectorSession"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
92e8cd3 to
4bc5d7e
Compare
pdabre12
left a comment
There was a problem hiding this comment.
Just two nits, otherwise LGTM.
4bc5d7e to
551c188
Compare
551c188 to
4f56709
Compare
The runtime should consolidate to the `ExpressionOptimizerProvider` factory so that it can be customized without significant refactoring.
aditi-pandit
left a comment
There was a problem hiding this comment.
+1 for presto-native-execution changes.
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
Release Notes