Centralize unparser select-scope boundary logic and add characterization tests#22683
Open
kosiew wants to merge 4 commits into
Open
Centralize unparser select-scope boundary logic and add characterization tests#22683kosiew wants to merge 4 commits into
kosiew wants to merge 4 commits into
Conversation
- Added `test_unparse_subquery_alias_select_scope_boundaries` to cover aggregate, window, sort, limit, and union under SubqueryAlias. - Added `test_unparse_window_over_sort_without_projection` to test window functions over sort without projection. - Added `test_unparse_window_over_union_without_projection` to test window functions over union without projection.
…ependent_select_scope - Introduced a private SelectScopeContext. - Centralized the plan_requires_independent_select_scope function. - Updated existing wrappers to delegate to: - requires_derived_subquery - window_input_requires_derived_subquery
- Removed private SelectScopeContext and dispatcher helper from the unparser. - Inlined exact matches in both callers for improved clarity. - Added row_number_over helper and removed redundant window expression setup in new test cases. - Inlined throwaway SQL variables in test snapshots to enhance readability.
…n unparser - Restored SelectScopeContext in the SQL unparser - Added central function plan_requires_independent_select_scope() - Made requires_derived_subquery() and window_input_requires_derived_subquery() delegate to both wrappers
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
The SQL unparser currently determines when a logical plan must preserve an independent
SELECTscope in multiple places, with different operator sets depending on context. While those differences are intentional, the underlying rationale is not centralized or documented, making future changes harder to reason about.This PR adds characterization tests for the current behavior and introduces a single helper that makes the context-specific boundary decisions explicit without changing generated SQL.
What changes are included in this PR?
Introduce a private
SelectScopeContextenum to describe the two select-scope boundary contexts:WindowInputSubqueryAliasChildAdd a centralized helper:
plan_requires_independent_select_scope(plan, context)Refactor:
window_input_requires_derived_subquery()requires_derived_subquery()so they delegate to the new helper while preserving existing behavior.
Add characterization tests covering
SubqueryAliaschildren that must remain derived:Add characterization tests covering
Windowinputs that must remain derived:Add a shared helper for constructing
ROW_NUMBER()window expressions used by the new tests.No SQL output changes are intended by this refactor.
Are these changes tested?
Yes.
New tests added in
datafusion/sql/tests/cases/plan_to_sql.rs:test_unparse_subquery_alias_select_scope_boundariestest_unparse_window_over_sort_without_projectiontest_unparse_window_over_union_without_projectionThese tests validate the current derived-subquery boundary behavior and ensure the refactor remains behavior-preserving.
Are there any user-facing changes?
No.
This change is a refactor and test expansion that preserves existing SQL generation behavior and does not introduce user-facing functionality changes.
LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed and tested.