-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Plan table function invocation with table arguments #14175
Conversation
5216363
to
5cc676b
Compare
fff6ce8
to
b2aefe8
Compare
c08453b
to
ba2d670
Compare
09dffde
to
8913a4b
Compare
334339f
to
5c5ce68
Compare
5c5ce68
to
2a29508
Compare
@@ -1886,6 +1924,10 @@ public NodeRepresentation addNode( | |||
List<PlanCostEstimate> estimatedCosts = allNodes.stream() | |||
.map(nodeId -> estimatedStatsAndCosts.getCosts().getOrDefault(nodeId, PlanCostEstimate.unknown())) | |||
.collect(toList()); | |||
String argumentName = argumentNames.get(rootNode.getId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing the whole map of node id -> name and having this method look it up, it would be cleaner to pass the intended name directly. At the end of the day, for each branch of recursion when visiting a child, there's only one name that's valid.
This means that the Context just just contain a single name that represents the "tag" associated with the subtree. If present, it will be prepended to the root of that subtree when printed. It also makes it more generic and allows for tagging any subtree (e.g., for a JOIN, we might want to tag the left and right differently), etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the context had just a single tag instead of the map, I would have to process sources in a loop, creating a new context per child instead of just call processChildren
.
However, my main reason for using a map by id was to make it more robust. The usual usage pattern for context is that we process the node, and then recursively process children, passing the same context. In the current design I can safely do it, as node ids are unique. If the context had just a tag, we would need to explicitly clear the context before the recursive call. It would be unintuitive for future contributors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the context had just a single tag instead of the map, I would have to process sources in a loop, creating a new context per child instead of just call processChildren.
That's ok. It would make the intention much clearer.
However, my main reason for using a map by id was to make it more robust. The usual usage pattern for context is that we process the node, and then recursively process children, passing the same context.
That's an abuse of what "context" in the case of the visitor is meant to do. The context is supposed to be specific to the node being visited (e.g., in filter pushdown, it contains the effective filter applied to the output of the current node). Sometimes, it's just easy to pass the same object, but that's not necessarily the best option in all cases.
If the context had just a tag, we would need to explicitly clear the context before the recursive call.
No, it'd be better to just create a new context with the proper tag.
private final boolean rowSemantics; | ||
private final boolean pruneWhenEmpty; | ||
private final boolean passThroughColumns; | ||
private final Specification specification; | ||
private final Optional<Specification> specification; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strange for a table function to use a WindowNode.Specification
. We should either move it out of WindowNode (preferable) or create a separate one (not ideal if they represent the same concept)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted Specification
.
2a29508
to
16c9f2f
Compare
@martint could you please take a look? |
16c9f2f
to
2cf93aa
Compare
@martint I changed the |
core/trino-main/src/main/java/io/trino/sql/planner/planprinter/PlanPrinter.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/plan/Specification.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/sql/planner/assertions/TableFunctionMatcher.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/sql/planner/assertions/TableFunctionMatcher.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/sql/planner/assertions/TableFunctionMatcher.java
Outdated
Show resolved
Hide resolved
2cf93aa
to
a38a039
Compare
a38a039
to
91bff67
Compare
It will be used to append table argument names to nodes being the sources of a table function.
91bff67
to
8bd1717
Compare
This PR adds the RelationPlanner part for table function invocation with table and descriptor arguments.
These types of arguments are not yet supported -- they cause the query to fail during the initial planning phase.
No docs or release notes required.
based on #14115
Example output from the
PlanPrinter
:query:
fragment of the plan: