adriangb opened a new issue, #25449: URL: https://github.com/apache/datafusion/issues/25449
### Is your feature request related to a problem or challenge? Two optimizer rules mark the expressions they create by putting a magic string at the front of an alias name. `CommonSubexprEliminate` uses `__common_expr`. `ExtractLeafExpressions` uses `__datafusion_extracted`. Later code then reads that name back to decide what the expression is. The sniffing sites: `datafusion/optimizer/src/extract_leaf_expressions.rs` - line 44, `const EXTRACTED_EXPR_PREFIX: &str = "__datafusion_extracted";` - `advance_generator_past_existing`: strips the prefix, parses the number after it, and advances the alias counter past it. - `routing_extract`: `starts_with(EXTRACTED_EXPR_PREFIX)` to skip an expression that is already extracted. - `build_extraction_projection_impl`: `starts_with(EXTRACTED_EXPR_PREFIX)` to collect the extractions a projection already carries. - `split_and_push_projection`: `starts_with(EXTRACTED_EXPR_PREFIX)` to split a projection into its extracted part and its user part. - `is_pure_extraction_projection`: `starts_with(EXTRACTED_EXPR_PREFIX)` on every expression, to decide whether the whole projection may be pushed without re-extraction. `datafusion/optimizer/src/common_subexpr_eliminate.rs` - line 41, `const CSE_PREFIX: &str = "__common_expr";` `datafusion/sql/src/statement.rs` - `check_plan` rejects any plan whose schema holds a field whose name starts with `__common_expr`, with "is a reserved DataFusion column name, please use another name". The protocol has already failed one time. https://github.com/apache/datafusion/pull/20432 fixed https://github.com/apache/datafusion/issues/20430: running the optimizer two times with two `AliasGenerator` instances produced a second `__datafusion_extracted_1`, and the plan failed with "Schema contains duplicate unqualified field name". The fix scans every expression of every node, including expressions inside subqueries, parses the number out of each matching alias, and seeds the generator above it. That scan runs on every plan, whether or not the rule does anything. The doc comment on the constant states the remaining risk: > This prefix is **reserved for internal optimizer use**. User-defined aliases starting with this prefix may be misidentified as optimizer-generated extraction aliases, leading to unexpected behavior. Do not use this prefix in user queries. A user alias is not the only source. Any producer of a `LogicalPlan` can write that name: a plan deserialized from proto, a plan built with the `LogicalPlanBuilder` API, a plan round-tripped through the unparser and re-planned. ### Describe the solution you'd like Carry the marker as data on the expression, not in its name. Option A, a flag on `Alias`. Add a field such as `origin: AliasOrigin`, with variants `User`, `CommonSubexpr` and `LeafExtraction`. `Alias` already carries `relation` and `metadata`, so the shape exists. Every `starts_with` above becomes a match on that field. `advance_generator_past_existing` is no longer needed, because a user alias can no longer be mistaken for a generated one, and the generator only has to be unique inside one plan. Option B, a dedicated `Expr` wrapper, for example `Expr::Internal(InternalAlias)`. This is a stronger statement and keeps `Alias` untouched. It is also a much wider change, because every `match` over `Expr` in the workspace has to handle the new variant. Option A looks like the better trade. ### Describe alternatives you've considered **Keep the prefix and make the check stricter**, for example a full regex on `__datafusion_extracted_[0-9]+$`. This narrows the accidental match. It does not stop a deliberate or a machine generated name, and it keeps the scan in `advance_generator_past_existing`. **Keep the prefix and reject it at the SQL front end**, as `check_plan` already does for `__common_expr`. This only covers plans that come from SQL text. ### Additional context Compatibility points that a change here has to answer. - **Proto.** `Alias` is serialized as `AliasNode` in `datafusion/proto-models/proto/datafusion.proto`, which today holds `expr`, `alias`, `relation` and `metadata`. A new field has to be optional, and an old reader that skips it has to keep the current behaviour. An old reader loses the flag on a round trip. This matters for the case that PR 20432 fixed, where a plan is optimized two times. - **Unparser.** `datafusion/sql/src/unparser` writes an alias as `expr AS name`. SQL has no place for the flag. A plan that goes out through the unparser and comes back in loses it. The unparser should probably refuse to emit a generated alias, or rename it. - **Display.** `EXPLAIN` output and every snapshot test print the alias name. Keeping the printed name as it is today, and keeping the same counter, keeps the snapshots stable. - **The reserved-name error.** `check_plan` in `datafusion/sql/src/statement.rs` can stay as it is. It would become a courtesy check rather than a correctness one. Size: medium. The mechanical part is the seven sniffing sites plus the `Alias` constructors. The compatibility part above is the work that needs a decision. Tracked in the leaf-pushdown EPIC. -- 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]
