presto icon indicating copy to clipboard operation
presto copied to clipboard
verified publisher icon prestodb

feat: TVF Part 7/X Final analyzer/planner/optimizer changes for tvf

Open mohsaka opened this issue 2 months ago • 6 comments

Description

This PR contains all final changes to SPI/analysis/planning/optimization. This PR does not include LocalExecutionPlanner changes or the exclude columns optimization which will come in future PRs.

Motivation and Context

Impact

Test Plan

Added new test cases.

Rule test cases: TestImplementTableFunctionSource TestPruneTableFunctionProcessorColumns TestPruneTableFunctionProcessorSourceColumns TestRemoveRedundantTableFunction

Planner test case: planner/TestTableFunctionInvocation

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

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 ==

mohsaka avatar Oct 27 '25 21:10 mohsaka

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

aditi-pandit avatar Nov 08 '25 04:11 aditi-pandit

@jaystarshot Thanks for the review. An end to end integration tests requires the changes in LocalExecutionPlanner and TestingTableFunctions which defines all the table functions used for testing. These change are included in commit https://github.com/prestodb/presto/pull/26188/commits/92fb07ef5c821b8ef2e6246cb1b2d1e9c488dcb5 that were orginally intended for a subsequent PR. I've now updated this PR to include that commit, along with some optimization changes from https://github.com/prestodb/presto/pull/26506 and https://github.com/prestodb/presto/pull/26188. This could provide a comprehensive view of the end-to-end code changes and test coverage.

Please let me know if this works well for your review, or if you have any other thoughts. Thanks!

cc @aditi-pandit

xin-zhang2 avatar Nov 13 '25 17:11 xin-zhang2

Thanks @xin-zhang2 ,will review this as the source of truth. I might need help reviewing some execution changes later since i would ooo mid next week

jaystarshot avatar Nov 13 '25 18:11 jaystarshot

@tdcmeehan Hi Tim, would you mind helping review this PR while Jay is ooo, if possible? Thanks in advance!

xin-zhang2 avatar Nov 14 '25 15:11 xin-zhang2

Did a rebase to resolve the conflicts. @jaystarshot Would mind continue reviewing this PR? Thanks!

xin-zhang2 avatar Dec 03 '25 16:12 xin-zhang2

Sorry I was ooo, checking again

jaystarshot avatar Dec 09 '25 15:12 jaystarshot

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.

mohsaka avatar Dec 12 '25 17:12 mohsaka

@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!

mohsaka avatar Dec 13 '25 08:12 mohsaka