zhuqi-lucas commented on code in PR #23682: URL: https://github.com/apache/datafusion/pull/23682#discussion_r3695699340
########## datafusion/sqllogictest/test_files/coalesce_first_last.slt: ########## @@ -0,0 +1,84 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at + +# http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Data with multiple groups, NULL value cells, and a unique ORDER BY key. +statement ok +CREATE TABLE t(p int, a int, b int, o int) AS VALUES Review Comment: The implementation logic is solid, but on reflection I'd like to hold this until the result-level coverage is stronger, so switching my review to request-changes on **tests only**. The unit tests assert *plan shape*; result-level semantic equivalence of the rewrite rests entirely on this file — which is currently one table + one query pair: `ASC`-only, int-only payload, and no NULLs in the ORDER BY key. For a rule that rewrites execution semantics, that leaves the highest-risk dimensions unexercised: | gap | risk | |---|---| | **NULL in the ORDER BY column** (incl. a group whose keys are all NULL) | highest — the window and aggregate paths must agree on null ordering; a mismatch is silently wrong results | | **DESC + explicit `NULLS FIRST` / `NULLS LAST`** | the reverse/null-option plumbing into `first_value` has no result check (the DESC unit test only asserts the plan) | | **multi-column ORDER BY** (`o1 ASC, o2 DESC`) | bucket key is `Vec<Sort>`; multi-key comparison has no result check | | **string / struct payload** | struct payload means `named_struct('c0', struct_col)` → a struct-of-struct unpacked by nested `get_field` — I don't think this shape is executed anywhere today | | **duplicate ORDER BY keys** | raised in my earlier comment (deterministically assertable by duplicating entire rows) | Each can follow the flag-off/flag-on same-results pattern this file already establishes, e.g.: ```sql -- NULL order keys + DESC + explicit null placement CREATE TABLE t2(p int, a int, b int, o int) AS VALUES (1, 10, 100, NULL), (1, 20, 200, 1), (1, 30, 300, 2), (2, 40, 400, NULL), (2, 50, 500, NULL); -- group 2: all-NULL keys SELECT p, first_value(a ORDER BY o DESC NULLS LAST), first_value(b ORDER BY o DESC NULLS LAST) FROM t2 GROUP BY p ORDER BY p; -- multi-key ordering first_value(a ORDER BY o1 ASC, o2 DESC), first_value(b ORDER BY o1 ASC, o2 DESC) -- string + struct payload (struct-of-struct + nested get_field) CREATE TABLE t3(p int, s varchar, st struct<x int, y varchar>, o int) AS ...; SELECT p, first_value(s ORDER BY o), first_value(st ORDER BY o) FROM t3 GROUP BY p; ``` With those in place (baseline vs enabled asserted identical for each) I'm happy to re-approve — the rewrite itself I've already traced and it's well-built. -- 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]
