Support table functions with pushdown to connector#11336
Support table functions with pushdown to connector#11336kasiafi merged 9 commits intotrinodb:masterfrom
Conversation
|
Sadly, not all jdbc-based connectors want to cooperate. Testing all to see what they need. |
There was a problem hiding this comment.
Ideally, the formatting of each argument should be done by a recursive call to format each element, instead of switching on the specific concrete classes here.
There was a problem hiding this comment.
Done for all except Expression arguments. I order to be processed recursively, they would require wrapping in another AST node, which I considered too much overhead.
There was a problem hiding this comment.
It'd be better to model these as sub-rules so that the construction occurs naturally by the recursive tree traversal instead of switching on whether qualifiedName is present or not
8c3cdc9 to
a05ddd6
Compare
149971a to
b8aa8f0
Compare
34b3d71 to
907158e
Compare
There was a problem hiding this comment.
Why is the argument a list of names?
There was a problem hiding this comment.
Refactored so that the list is implementation detail in table function resolution.
core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/metadata/TableFunctionRegistry.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
There should be explicit validation to make sure the arguments provided to the function are not duplicated. This will catch that condition, but with an unclear error message.
There was a problem hiding this comment.
applyFunction should return a TableHandle and a list of ColumnHandle for that TH corresponding to the columns coming out of the original table function. This optimizer should not have to derive that from the table metadata, which may not actually exist for a "virtual" table.
There was a problem hiding this comment.
Refactored. Now the TableFunctionHandle holds the catalog name.
3325323 to
0acafc1
Compare
|
@martint Applied comments |
core/trino-main/src/main/java/io/trino/metadata/TableFunctionRegistry.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/metadata/TableFunctionHandle.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/ptf/ConnectorTableFunction.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/ptf/ConnectorTableFunction.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
As a follow up - can we extract it to a common util method ?
core/trino-spi/src/main/java/io/trino/spi/ptf/TableArgument.java
Outdated
Show resolved
Hide resolved
7ddb7ae to
45cb337
Compare
eec1b22 to
a2139f7
Compare
martint
left a comment
There was a problem hiding this comment.
Great job. Let's merge everything in except for the functions added to the jdbc and elasticsearch connectors. We need to come up with a good name for them and make sure they have reasonable semantics -- I haven't had a chance to review them yet.
core/trino-main/src/main/java/io/trino/metadata/TableFunctionHandle.java
Outdated
Show resolved
Hide resolved
Preparatory commit for supporting table functions in JDBC connectors
|
Woah, nice job @kasiafi! |
|
I am going to assume this needs to release notes entry or docs at this stage. Going forward can you maybe use the PR template to indicate that @kasiafi .. thanks |
@mosabua this does need a release notes entry and docs. I'll add the proposed release notes entry in the relnotes issue. I'm currently working on the documentation -- it's likely we'll have to wait for it until the next release. |
#1839
This PR introduces partial support for Table Functions (including Polymorphic Table Functions).
Temporarily, only constant scalar arguments are supported (no Table or Descriptor arguments).
The supported execution path for a Table Function is to be pushed down into connector, and replaced with a TableHandle.
Later, Descriptor and Table arguments will be supported, and operator-based execution will be enabled for Table Functions.