Vanzeren commented on PR #25803: URL: https://github.com/apache/datafusion/pull/25803#issuecomment-5857986635
Addressed all four points in 076b8d89d. 1. `expr.rs:415` — removed the "rather than dropping the ordering" aside. 2. `expr.rs:434` — now branches on `order_by.is_empty()` first, with the two spellings nested beneath it. 3. `plan_to_sql.rs:4628` — the doc comment now describes the two syntax variants on their own terms, without referring to the bug. 4. `plan_to_sql.rs:4717` — deleted the bespoke re-planning test and moved the SQL into `roundtrip_statement`. That is a stronger check than what I had: it asserts `LogicalPlan` equality rather than comparing `display_indent()` text. While moving it I confirmed the coverage actually transfers, by reverting `expr.rs` to `5a09d99` and running `roundtrip_statement`: ``` unparsed : SELECT person.first_name, array_agg(person.age) FROM person GROUP BY person.first_name raw eq : false # plan == plan_roundtrip norm eq : false # after remove_column_self_aliases ``` The test fails in that state for both `array_agg(age ORDER BY salary)` and `last_value(age ORDER BY salary)`, so the case is still guarded from its new location. Also run: `cargo test -p datafusion-sql` (597 + 92 + 12 pass), the core unparser tests including the TPC-H and ClickBench round-trips, `cargo fmt`, and `cargo clippy --all-targets --all-features -- -D warnings`. -- 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]
