Repository navigation
Conversation
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for addressing the repeated USING-column discovery during SELECT planning. Sharing the result across explicit projection expressions is an appropriate optimization, and the new benchmark targets the wide, deeply nested queries described in the issue.
The architectural boundary is sound: USING-column discovery depends on the input plan, not on individual expressions, so computing it once per projection avoids redundant traversal without introducing persistent caching or changing public APIs. However, moving discovery unconditionally to the projection level also makes wildcard-only queries perform unnecessary full-plan traversals.
I'm requesting changes to preserve the existing wildcard fast paths while retaining the explicit-expression speedup. Please make discovery conditional or lazy and validate the qualified-wildcard case against the parent revision. The additional semantic and scaling coverage suggestions are non-blocking.
SqlToRel::sql_select_to_rex called LogicalPlan::using_columns() for every SELECT item, walking the whole input plan each time. Compute it once in prepare_select_exprs (and once for a wildcard's REPLACE items) and pass it down. Adds a nested wide SELECT case to the sql_planner benchmark.
Wildcard-only projections never normalize an expression, so they no longer walk the input plan for USING columns. REPLACE items share the same lazily collected set. Adds a width/depth matrix and a qualified-wildcard control to the nested SELECT benchmark, and an SLT for REPLACE over a USING join. Co-Authored-By: Claude Opus 5.5 <[email protected]>
e6789df to
3e532c4
Compare
|
thanks so much for the review @kosiew. I addressed your comments (with AI assistance) |
There was a problem hiding this comment.
Thanks for the follow-up changes. The projection-local lazy cache appropriately eliminates redundant USING-column traversals while preserving expression normalization and wildcard REPLACE behavior. The added regression coverage and nested-projection benchmarks address the relevant correctness and performance concerns.
The ownership boundary is appropriate: USING-column metadata belongs to the SELECT input plan and can be shared across its projection expressions. Lazy initialization also avoids unnecessary collection for wildcard-only projections, while passing the same cache through REPLACE planning preserves consistent resolution semantics.
All previous comments have been addressed, including the wildcard-traversal concern. I found no remaining blockers or actionable suggestions.
Which issue does this PR close?
LogicalPlan::using_columns()once per SELECT item, making wide nested SELECTs quadratic #26313.Rationale for this change
Planning a SELECT costs O(items × input plan size), because
SqlToRel::sql_select_to_rexcallsLogicalPlan::using_columns()once per SELECT item. Each call walks the whole input plan, subqueries and expressions included, and the result depends only on the input plan. Nested wide SELECTs (views over views) therefore plan in roughly O(width² × depth²). The issue has the numbers. At 4000 columns × 12 levels,statement_to_plangoes from 8.0 s to 4.4 s with this change alone, and to 0.6 s once theDFSchemascan from #26314 is also gone.What changes are included in this PR?
prepare_select_exprscomputesplan.using_columns()once and passes it tosql_select_to_rex.plan_wildcard_optionsdoes the same for a wildcard'sREPLACEitems.sql_plannerbenchmark case,logical_nested_select_700_columns_8_levels. On an M1 Max its query goes from 178 ms to 122 ms instatement_to_plan, measured with a standalone harness because the full benchmark needs the clickbench data.One behaviour difference: if
using_columns()itself fails, the error now surfaces once for the SELECT instead of once per item throughDataFusionErrorBuilder.using_columnsonly errors if the tree walk does, so I don't expect this to be observable, but I can keep the old shape with a lazily filledOptionif you'd prefer.What is the testing strategy for this PR?
The planned output is unchanged, so this relies on the existing
datafusion-sqltests and sqllogictests, which pass locally. Those cover USING / NATURAL joins and wildcard expansion over them, the casesusing_columnsexists for. The new benchmark case covers the performance side.Are there any user-facing changes?
No. Planning is faster and plans are identical.
Prepared with AI assistance (Claude Code).