feat: TVF Part 7/X Final PR of adaptation changes by mohsaka · Pull Request #26445 · prestodb/presto

mohsaka · 2025-10-27T21:27:55Z

Description

This PR contains all final changes to TVF functionality.

Motivation and Context

Completes the addition of TVF support in Presto.

Impact

Test Plan

Added new test cases.

Rule test cases:
TestTransformTableFunctionToTableFunctionProcessor
TestPruneTableFunctionProcessorColumns
TestPruneTableFunctionProcessorSourceColumns
TestRemoveRedundantTableFunction
TestRewriteExcludeColumnsFunctionToProjection

Planner test case:
planner/TestTableFunctionInvocation

System TVF Test Cases:
TestExcludeColumnsFunction
TestSequenceFunction

Re-ran previous test cases to check for regressions:
test/TestTableFunctionInvocation
TestTableFunctionRegistry
TestAnalyzer

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.
If adding new dependencies, verified they have an OpenSSF Scorecard score of 5.0 or higher (or obtained explicit TSC approval for lower scores).

Release Notes

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

== NO RELEASE NOTE ==

sourcery-ai

Sorry @mohsaka, your pull request is larger than the review limit of 150000 diff characters

sourcery-ai

Sorry @mohsaka, your pull request is larger than the review limit of 150000 diff characters

aditi-pandit

Thanks @mohsaka.

It might get easier to review the code if we split as follows :
i) Changes in TestingTableFunctions.java
ii) Add the TableFunctionNode changes and the basic structural wiring (in Printer, GraphVizPrinter, PlanBuilder, PlanMatcher)
iii) Similar changes ffor TableFunctionProcessorNode..The PlanNode definition and the basic structural wiring like above.
iv) There are bunch of changes like those in Field.java, StatementAnalyzer changes which can be one off changes and can be made independent of this PR.
v) Basic Planner rule for ImplementTableFunctionSource
vi) PruneTableFunction* rules + TestPrune*
vii) Remove* rules + TestRemove*

aditi-pandit · 2025-10-31T06:04:35Z

presto-main-base/src/main/java/com/facebook/presto/testing/LocalQueryRunner.java

Move this change to a separate PR.

aditi-pandit · 2025-10-31T06:16:05Z

presto-main-base/src/main/java/com/facebook/presto/sql/planner/QueryPlanner.java

Don't understand the reason for the changes in this file. Why did you have to make this class public ?

Coerce function needed for RelationPlanner

QueryPlanner.PlanAndMappings copartitionCoercions = partitionQueryPlanner.coerce(sourcePlanBuilder, partitioningColumns, analysis, idAllocator, variableAllocator, metadata); sourcePlanBuilder = copartitionCoercions.getSubPlan(); partitionBy = partitioningColumns.stream() .map(copartitionCoercions::get) .collect(toImmutableList());

aditi-pandit · 2025-10-31T06:45:18Z

presto-main-base/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

                Scope inputScope = analysis.getScope(tableArgumentsByName.get(name).getRelation());
                columns.stream()
-                        .filter(column -> column < 0 || column >= inputScope.getRelationType().getAllFieldCount()) // hidden columns can be required as well as visible columns
+                        .filter(column -> column < 0 || column >= inputScope.getRelationType().getVisibleFieldCount())


This can be added as a single change without the others.

aditi-pandit

This code looks good. Will do one more read.

aditi-pandit · 2025-11-08T03:20:35Z

...main-base/src/main/java/com/facebook/presto/sql/planner/plan/TableFunctionProcessorNode.java

+    // for processing or for pass-through. null value in the marker column indicates that the value at the same
+    // position in the source column should not be processed or passed-through.
+    // the mapping is only present if there are two or more sources.
+    private final Optional<Map<VariableReferenceExpression, VariableReferenceExpression>> markerVariables;


Can you add a comment with an example for this.

@aditi-pandit Thanks. I've added an example in the comment.

aditi-pandit · 2025-11-08T03:28:00Z

presto-main-base/src/main/java/com/facebook/presto/sql/planner/TableFunctionUtils.java

+        return orderBy;
+    }
+
+    static void addPassthroughColumns(ImmutableList.Builder<VariableReferenceExpression> outputVariables,


Can you add a comment about the usage of this function and explanation for each parameter

@aditi-pandit Thanks. I've added a comment for the explannation.

aditi-pandit · 2025-11-08T04:07:24Z

@jaystarshot : PTAL. This code has the main planner changes for Table function.

jaystarshot · 2025-11-12T18:08:06Z

...c/main/java/com/facebook/presto/sql/planner/iterative/rule/ImplementTableFunctionSource.java

+ *                          - source T2(a2, b2)
+ * </pre>
+ */
+public class ImplementTableFunctionSource


the naming of this rule is non intuitive, can it be improved? eg
TransformTableFunctionToProcessorNodeRule

@jaystarshot Thanks. I've renamed it to TransformTableFunctionToTableFunctionProcessor. What do you think of this name?

jaystarshot

I think this PR is missing end to end integration (execution) tests, without those its hard to say if the addExchanges etc is correct. Can you please add them

jaystarshot

I have reviewed most of the planner changes only "SymbolMapper.java" is left which i will review.

jaystarshot · 2025-12-10T22:36:01Z

...e/src/main/java/com/facebook/presto/sql/planner/optimizations/StreamPropertyDerivations.java

+                return Optional.empty();
+            });
+
+            return translatedProperties.unordered(true);


nit: maybe a comment here explaining why conservative in being unordered

Added comment. Main reason is that the user can pretty much do anything with the rows they are provided. So we don't have a guarantee that they are ordered after table function application.

jaystarshot · 2025-12-10T22:45:32Z

...ase/src/main/java/com/facebook/presto/sql/planner/optimizations/UnaliasSymbolReferences.java

+                PlanNode newSource = node.getSources().get(i).accept(this, context);
+                newSources.add(newSource);
+
+                SymbolMapper inputMapper = new SymbolMapper(new HashMap<>(), warningCollector);


This input mapper is empty, this doesn't look correct

This was pretty wrong. Fixed a few things.

Mapper should be used from Rewriter mapping.

We should have been using context.rewrite instead of accept. Following the convention of the other Nodes.

Thanks!

mohsaka · 2025-12-12T17:34:02Z

I have reviewed most of the planner changes only "SymbolMapper.java" is left which i will review.

Thank you for the review! I will take a look today or monday.

tdcmeehan · 2025-12-12T18:52:38Z

...to-spi/src/main/java/com/facebook/presto/spi/function/table/TableFunctionProcessorState.java

+    final class Processed
+            implements TableFunctionProcessorState
+    {
+        private final boolean usedInput;


Can you document what usedInput signifies so folks can understand the semantics?

Added comment

tdcmeehan · 2025-12-12T20:28:27Z

presto-spi/src/main/java/com/facebook/presto/spi/function/table/TableFunctionDataProcessor.java

+     * @param input a tuple of {@link Page} including one page for each table function's input table.
+     * Pages list is ordered according to the corresponding argument specifications in {@link ConnectorTableFunction}.
+     * A page for an argument consists of columns requested during analysis (see {@link TableFunctionAnalysis#getRequiredColumns()}}.
+     * If any of the sources is fully processed, {@code Optional.empty)()} is returned for that source.


Suggested change

* If any of the sources is fully processed, {@code Optional.empty)()} is returned for that source.

* If any of the sources is fully processed, {@code Optional.empty()} is returned for that source.

tdcmeehan · 2025-12-12T20:28:39Z

...to-spi/src/main/java/com/facebook/presto/spi/function/table/TableFunctionSplitProcessor.java

+    /**
+     * This method processes a split. It is called multiple times until the whole output for the split is produced.
+     *
+     * @param split a {@link ConnectorSplit} representing a subtask.


Let's document that this is Nullable and when it's expected to be null.

Added description for when table function is labeled KEEP WITH EMPTY and has no input.

tdcmeehan · 2025-12-12T20:30:15Z

...to-spi/src/main/java/com/facebook/presto/spi/function/table/TableFunctionSplitProcessor.java

+public interface TableFunctionSplitProcessor
+{
+    /**
+     * This method processes a split. It is called multiple times until the whole output for the split is produced.


Should we make it clear this is 1:1 with a split?

Added comment explaining that the Split processor is one to one with a split.

tdcmeehan · 2025-12-12T21:10:06Z

presto-main-base/src/main/java/com/facebook/presto/operator/table/Sequence.java

+import static com.google.common.base.Preconditions.checkArgument;
+import static java.lang.String.format;
+
+public class Sequence


Let's add documentation for this. This probably entails a new section for TVFs, next to our existing functions.

Agreed, we plan on doing something similar to
https://trino.io/docs/current/functions/table.html

tdcmeehan · 2025-12-12T21:10:21Z

presto-main-base/src/main/java/com/facebook/presto/operator/table/ExcludeColumns.java

+import static java.util.Locale.ENGLISH;
+import static java.util.stream.Collectors.joining;
+
+public class ExcludeColumns


We should add documentation for this as well.

Ditto to above.
https://trino.io/docs/current/functions/table.html

tdcmeehan · 2025-12-12T21:14:51Z

presto-main-base/src/main/java/com/facebook/presto/operator/TableFunctionOperator.java

+
+    private final OperatorContext operatorContext;
+
+    private final PageBuffer pageBuffer = new PageBuffer();


My IDE is showing this is unused.

I'm not too sure on why this was changed from the old implementation. But switched back to the old one which was simpler.

tdcmeehan · 2025-12-12T21:29:35Z

presto-main-base/src/main/java/com/facebook/presto/metadata/HandleResolver.java

+            // Fallback if needed
+            return getFunctionId(split, tableFunctionSplitResolvers);


Can you explain why we need this Exception-based fallback? Is there any way to refactor to avoid the need to do this? It's far preferable to just let Exceptions bubble up.

tdcmeehan · 2025-12-12T21:37:50Z

presto-main-base/src/main/java/com/facebook/presto/operator/table/ExcludeColumns.java

+                    .map(RowType.Field::getName)
+                    .filter(Optional::isPresent)
+                    .map(Optional::get)
+                    .map(name -> name.toLowerCase(ENGLISH))


I think this will break any connector that has case sensitive identifiers.

Agreed. There's a comment left by kasiafi acknowledging this issue and a TODO above.

// column names in DescriptorArgument are canonical wrt SQL identifier semantics. // column names in TableArgument are not canonical wrt SQL identifier semantics, as they are taken from the corresponding RelationType. // because of that, we match the excluded columns names case-insensitive // TODO: apply proper identifier semantics

tdcmeehan · 2025-12-12T21:39:34Z

presto-main-base/src/main/java/com/facebook/presto/operator/PageBuffer.java

+import static com.google.common.base.Preconditions.checkState;
+import static java.util.Objects.requireNonNull;
+
+public class PageBuffer


This looks completely unused.

Used now. Not sure why it was not used before. I'm guessing I missed something when bringing code in by component.

mohsaka · 2025-12-13T08:42:02Z

@jaystarshot @tdcmeehan Thank you for the thorough reviews. Really appreciate the time you take to do them. I have addressed the comments so please take a second look when you have the chance. Thanks again!

…nPlanner and ExcludeColumns optimizer rule. Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com> Co-authored-by: Xin Zhang <desertsxin@gmail.com>

Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com> Co-authored-by: mohsaka <135669458+mohsaka@users.noreply.github.com>

Changes adapted from trino/PR#16584 Author: kasiafi Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com> Co-authored-by: Xin Zhang <desertsxin@gmail.com>

Changes adapted from trino/PR#16716 Author: kasiafi Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>

Changes adapted from trino/PR#25493 Author: kasiafi Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>

…unctionProcessor

aditi-pandit · 2025-12-18T03:57:48Z

...cebook/presto/sql/planner/iterative/rule/TransformTableFunctionToTableFunctionProcessor.java

+        // as if it was a single partition. Alternatively, it could be split into smaller partitions of arbitrary size.
+        DataOrganizationSpecification specification = argumentProperties.getSpecification().orElse(UNORDERED_SINGLE_PARTITION);
+
+        PlanNode innerWindow = new WindowNode(


@mohsaka : We don't need to do window over window as a window node can have 2 window functions that have the same partition by and order by. Can you try combining the 2 windows with a single function list and see what happens ?

@mohsaka : I'm fine with doing this as a follow up PR.

@aditi-pandit We originally had it as a single window function, which can be viewed in one of our really old PR's
https://github.com/mohsaka/presto/blob/c2d64577387f41bd0f1270c55b0fa851920eb6bb/presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/ImplementTableFunctionSource.java#L358

However it caused us to modify the WindowFilterPushDown rule and remove this check

presto/presto-main-base/src/main/java/com/facebook/presto/sql/planner/optimizations/WindowFilterPushDown.java

Line 119 in c2b0f5f

checkState(node.getWindowFunctions().size() == 1, "WindowFilterPushdown requires that WindowNodes contain exactly one window function");

We didn't think this was proper, so we had to split it in 2.

tdcmeehan

Execution side LGTM

jaystarshot

Planning changes LGTM

prestodb-ci · 2026-01-02T21:34:21Z

@mohsaka imported this issue as lakehouse/presto #26445

prestodb-ci added the from:IBM PR from IBM label Oct 27, 2025

mohsaka changed the title ~~Final analyzer/planner/optimizer changes for tvf~~ feat: Final analyzer/planner/optimizer changes for tvf Oct 27, 2025

sourcery-ai bot reviewed Oct 27, 2025

View reviewed changes

mohsaka force-pushed the tvf_analyzer_final branch from 056ebac to 4f63c71 Compare October 27, 2025 23:06

mohsaka changed the title ~~feat: Final analyzer/planner/optimizer changes for tvf~~ feat: TVF Part 7/X Final analyzer/planner/optimizer changes for tvf Oct 27, 2025

mohsaka requested a review from aditi-pandit October 27, 2025 23:27

mohsaka marked this pull request as ready for review October 27, 2025 23:27

mohsaka requested review from a team, feilong-liu, jaystarshot and vivek-bharathan as code owners October 27, 2025 23:27

prestodb-ci requested review from a team and nmahadevuni and removed request for a team October 27, 2025 23:27

sourcery-ai bot reviewed Oct 27, 2025

View reviewed changes

aditi-pandit reviewed Oct 31, 2025

View reviewed changes

mohsaka force-pushed the tvf_analyzer_final branch from 4f63c71 to 95fd75c Compare October 31, 2025 19:14

mohsaka mentioned this pull request Oct 31, 2025

feat: TVF Part 6.5/X Small TVF analyzer/planning changes #26505

Closed

7 tasks

aditi-pandit reviewed Nov 8, 2025

View reviewed changes

xin-zhang2 force-pushed the tvf_analyzer_final branch 4 times, most recently from a952a34 to 33da439 Compare November 11, 2025 10:57

jaystarshot reviewed Nov 12, 2025

View reviewed changes

xin-zhang2 force-pushed the tvf_analyzer_final branch from 33da439 to f5e5c48 Compare November 13, 2025 16:05

xin-zhang2 requested review from a team, elharo and shrinidhijoshi as code owners November 13, 2025 16:05

tdcmeehan self-assigned this Dec 10, 2025

jaystarshot reviewed Dec 10, 2025

View reviewed changes

tdcmeehan self-requested a review December 12, 2025 19:14

tdcmeehan requested changes Dec 12, 2025

View reviewed changes

mohsaka requested review from jaystarshot and tdcmeehan December 13, 2025 08:40

mohsaka force-pushed the tvf_analyzer_final branch from f29548e to b84a071 Compare December 13, 2025 08:43

mohsaka and others added 10 commits December 13, 2025 10:56

Final analyzer/planner/optimizer changes for tvf except LocalExecutio…

80dca59

…nPlanner and ExcludeColumns optimizer rule. Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com> Co-authored-by: Xin Zhang <desertsxin@gmail.com>

Add optimizer rules for table functions.

1e39489

Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com> Co-authored-by: mohsaka <135669458+mohsaka@users.noreply.github.com>

Support execution by operator for table functions

2af2563

Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com> Co-authored-by: mohsaka <135669458+mohsaka@users.noreply.github.com>

Add table function exclude_columns

fe5ab94

Changes adapted from trino/PR#16584 Author: kasiafi Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com> Co-authored-by: Xin Zhang <desertsxin@gmail.com>

Add table function sequence

f33b4c6

Changes adapted from trino/PR#16716 Author: kasiafi Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>

Rewrite exclude_columns table function into projection

2b8142f

Changes adapted from trino/PR#25493 Author: kasiafi Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>

Rename ImplementTableFunctionSource to TransformTableFunctionToTableF…

b164cba

…unctionProcessor

fix testAllMethodsOverridden

d6f81b8

Address some review comments

8fe4d91

update PlanOptimizers

8170eaa

mohsaka force-pushed the tvf_analyzer_final branch from b84a071 to 3e548ad Compare December 13, 2025 18:57

Address comments

756b350

mohsaka force-pushed the tvf_analyzer_final branch from 3e548ad to 756b350 Compare December 14, 2025 17:58

aditi-pandit reviewed Dec 18, 2025

View reviewed changes

tdcmeehan reviewed Dec 19, 2025

View reviewed changes

jaystarshot reviewed Dec 19, 2025

View reviewed changes

tdcmeehan approved these changes Dec 19, 2025

View reviewed changes

jaystarshot approved these changes Dec 22, 2025

View reviewed changes

mohsaka changed the title ~~feat: TVF Part 7/X Final analyzer/planner/optimizer changes for tvf~~ feat: TVF Part 7/X Final PR of adaptation changes Jan 2, 2026

mohsaka merged commit c2b0f5f into prestodb:master Jan 2, 2026
85 of 93 checks passed

	* If any of the sources is fully processed, {@code Optional.empty)()} is returned for that source.
	* If any of the sources is fully processed, {@code Optional.empty()} is returned for that source.


		private final OperatorContext operatorContext;

		private final PageBuffer pageBuffer = new PageBuffer();

		// Fallback if needed
		return getFunctionId(split, tableFunctionSplitResolvers);

Conversation

mohsaka commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit commented Nov 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaystarshot left a comment

Choose a reason for hiding this comment

Uh oh!

jaystarshot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohsaka commented Dec 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohsaka commented Oct 27, 2025 •

edited

Loading

aditi-pandit Dec 18, 2025 •

edited

Loading