kosiew commented on code in PR #26317:
URL: https://github.com/apache/datafusion/pull/26317#discussion_r4492030722
##########
datafusion/sql/src/select.rs:
##########
@@ -1130,13 +1138,15 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
rename: options.opt_rename,
};
if let Some(replace) = options.opt_replace {
+ let using_columns = plan.using_columns()?;
Review Comment:
As an optional regression test, could you add a compact SLT combining
multiple REPLACE expressions with a USING join, asserting both replacement
values and the deduplicated join key? Existing tests cover these features
separately, but their combination would specifically protect the newly shared
normalization input.
##########
datafusion/core/benches/sql_planner.rs:
##########
@@ -850,6 +850,26 @@ fn criterion_benchmark(c: &mut Criterion) {
});
}
+ // Nested SELECTs that each name every column of the one below
+ {
+ let columns = |qualifier: &str| {
+ (0..700)
+ .map(|i| format!("{qualifier}.c{i} AS c{i}"))
+ .collect::<Vec<_>>()
+ .join(", ")
+ };
+ let mut query = format!("SELECT {} FROM t700", columns("t700"));
+ for level in 0..8 {
+ query = format!(
+ "SELECT {} FROM ({query}) l{level}",
+ columns(&format!("l{level}"))
+ );
+ }
Review Comment:
For future scaling coverage, consider adding a small width/depth matrix with
a shallow control while retaining this benchmark. A qualified-wildcard-only
variant would also help validate the traversal regression noted above. Keep SQL
generation outside the timed closure and compare identical cases on parent and
candidate without timing assertions in correctness tests.
##########
datafusion/sql/src/select.rs:
##########
@@ -1003,9 +1003,16 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
) -> Result<Vec<SelectExpr>> {
let mut prepared_select_exprs = Vec::with_capacity(projection.len());
let mut error_builder = DataFusionErrorBuilder::new();
+ let using_columns = plan.using_columns()?;
Review Comment:
The unconditional `using_columns()` call adds full-plan traversal to
wildcard-only projections that previously avoided it. For nested `SELECT q.*`
wrappers, this can add O(W × D²) expression visits; plain `SELECT *` also
duplicates discovery already performed during expansion. Please compute the
shared value only when an `UnnamedExpr` or `ExprWithAlias` needs it, or
initialize it lazily, while preserving discovery for REPLACE expressions.
Validate the explicit-expression speedup against a qualified-wildcard-only
control using traversal counts or comparable parent/candidate timings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
