jonmmease commented on code in PR #22998: URL: https://github.com/apache/datafusion/pull/22998#discussion_r4230736536
########## datafusion/expr/src/utils.rs: ########## Review Comment: If we want the output column order to be independent of alphabetic ordering and preserve the LHS schema order, this could be something like ```rust cols.sort_by_key(|c| plan.schema().maybe_index_of_column(c)); ``` ########## datafusion/sqllogictest/test_files/join_using_merged_key.slt: ########## @@ -0,0 +1,234 @@ +# 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. + +# Merged key of USING / NATURAL joins. +# +# The single merged key column exposed by a USING / NATURAL join equals +# COALESCE(left.k, right.k): for an outer join a row present on only one side +# must expose the key from the present side, not NULL. This matters for +# RIGHT / FULL joins, whose left key is NULL-padded on right-only rows. +# +# Regression coverage for https://github.com/apache/datafusion/issues/22881, +# where the merged key was resolved to the left column unconditionally and so +# came out NULL for right-only rows of RIGHT / FULL joins. +# +# Tables: a has keys {1,2,3}, b has keys {2,3,4}. +# matched: 2, 3 ; left-only: 1 ; right-only: 4 + +statement ok +create table a(k int, x int) as values (1, 10), (2, 20), (3, 30); + +statement ok +create table b(k int, y int) as values (2, 200), (3, 300), (4, 400); + +########## +# Already correct: INNER / LEFT (left key is never NULL-padded) +########## + +query I nosort +SELECT k FROM a INNER JOIN b USING (k) ORDER BY k +---- +2 +3 + +query I nosort +SELECT k FROM a LEFT JOIN b USING (k) ORDER BY k +---- +1 +2 +3 + +########## +# The bug: RIGHT / FULL merged key must come from the preserved side +########## + +query I nosort +SELECT k FROM a RIGHT JOIN b USING (k) ORDER BY k NULLS LAST +---- +2 +3 +4 + +query I nosort +SELECT k FROM a FULL JOIN b USING (k) ORDER BY k NULLS LAST +---- +1 +2 +3 +4 + +query I nosort +SELECT k FROM a NATURAL RIGHT JOIN b ORDER BY k NULLS LAST +---- +2 +3 +4 + +query I nosort +SELECT k FROM a NATURAL FULL JOIN b ORDER BY k NULLS LAST +---- +1 +2 +3 +4 + +########## +# Downstream of the merged key: WHERE must see the coalesced value, not the +# wrong NULL (ORDER BY is covered separately below) +########## + +# right-only row (k = 4) must be findable by its merged key +query III nosort +SELECT k, x, y FROM a FULL JOIN b USING (k) WHERE k = 4 +---- +4 NULL 400 + +########## +# Guards / reference: these are already correct and must stay correct +########## + +# qualified access to each side is independent of the merged key +query II nosort +SELECT a.k, b.k FROM a FULL JOIN b USING (k) ORDER BY coalesce(a.k, b.k) +---- +1 NULL +2 2 +3 3 +NULL 4 + +# the merged key is distinct from an explicitly qualified side key +query II rowsort +SELECT k, a.k FROM a LEFT JOIN b USING (k) +---- +1 1 +2 2 +3 3 + +query II rowsort +SELECT k, b.k FROM a RIGHT JOIN b USING (k) +---- +2 2 +3 3 +4 4 + +query III rowsort +SELECT k, a.k, b.k FROM a FULL JOIN b USING (k) +---- +1 1 NULL +2 2 2 +3 3 3 +4 NULL 4 + +# the explicit form is the reference for what the merged key should equal +query I nosort +SELECT coalesce(a.k, b.k) AS k FROM a FULL JOIN b ON a.k = b.k ORDER BY k +---- +1 +2 +3 +4 + +########## +# Regression: ORDER BY a *qualified* key while selecting the FULL merged key. +# The merged k is COALESCE(a.k, b.k), exposed unqualified; ORDER BY a.k folds +# the qualified a.k into the same projection. This must plan (it regressed to a +# schema-ambiguity error) and sort by a.k, not by the merged key: the right-only +# row (a.k IS NULL) sorts last, the rest descend by a.k -- so the output differs +# from ORDER BY k DESC (which would be 4, 3, 2, 1). +########## + +query I nosort +SELECT k FROM a FULL JOIN b USING (k) ORDER BY a.k DESC NULLS LAST +---- +3 +2 +1 +4 + +query I nosort +SELECT k FROM a NATURAL FULL JOIN b ORDER BY a.k DESC NULLS LAST +---- +3 +2 +1 +4 + +########## +# Regression: a *qualified* key and the *unqualified* merged key together in one +# ORDER BY (`ORDER BY a.k DESC NULLS LAST, k`). On main this was legal (the merged +# key resolved to a.k, so both sort keys were a.k); the merged-key fix must keep +# it legal -- renaming the merged key to dodge the `{k, a.k}` collision must also +# rewrite the unqualified `k` reference in the sort. a.k is unique here so the +# secondary `k` never breaks a tie, but the shape still exercises that path. +########## + +query I nosort +SELECT k FROM a FULL JOIN b USING (k) ORDER BY a.k DESC NULLS LAST, k +---- +3 +2 +1 +4 + +########## +# Wildcard over the join: the merged key must appear once and carry the right +# value. (commit 2 target -- the materialized merged column must dedupe under *) +########## + +query III nosort +SELECT * FROM a RIGHT JOIN b USING (k) ORDER BY k NULLS LAST +---- +2 20 200 +3 30 300 +4 NULL 400 + +query III nosort +SELECT * FROM a FULL JOIN b USING (k) ORDER BY k NULLS LAST +---- +1 10 NULL +2 20 200 +3 30 300 +4 NULL 400 + Review Comment: May be useful to add a test for a case with aliases in the opposite alphabetical order, like ```sql SELECT * FROM a AS z LEFT JOIN b AS y USING (k) ``` -- 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]
