spark-test commented on code in PR #59917:
URL: https://github.com/apache/spark/pull/59917#discussion_r4416457082
##########
sql/core/src/test/resources/sql-tests/inputs/cte-recursion.sql:
##########
@@ -832,3 +832,297 @@ WITH RECURSIVE t AS (
WHERE n <> m
)
SELECT * FROM t;
+
+-- Recursive CTE self-reference under INNER JOIN ... USING
+WITH RECURSIVE r(level, data) AS (
+ VALUES (0, 0)
+ UNION ALL
+ SELECT level + 1, data
+ FROM r INNER JOIN (VALUES (0)) AS t(data) USING (data)
+ WHERE level < 2
+)
+SELECT * FROM r;
+
+-- Recursive CTE self-reference under NATURAL INNER JOIN
+WITH RECURSIVE r(level, data) AS (
+ VALUES (0, 0)
+ UNION ALL
+ SELECT level + 1, data
+ FROM r NATURAL INNER JOIN (VALUES (0)) AS t(data)
+ WHERE level < 2
+)
+SELECT * FROM r;
+
+-- Recursive CTE self-reference on the right side of INNER JOIN ... USING and
NATURAL INNER JOIN
+WITH RECURSIVE r(level, data) AS (
+ VALUES (0, 0)
+ UNION ALL
+ SELECT level + 1, data
+ FROM (VALUES (0)) AS t(data) INNER JOIN r USING (data)
+ WHERE level < 2
+)
+SELECT * FROM r;
+
+WITH RECURSIVE r(level, data) AS (
+ VALUES (0, 0)
+ UNION ALL
+ SELECT level + 1, data
+ FROM (VALUES (0)) AS t(data) NATURAL INNER JOIN r
+ WHERE level < 2
+)
+SELECT * FROM r;
+
+-- The other side's non-key columns stay visible after the USING join
+WITH RECURSIVE r(level, data) AS (
+ VALUES (0, 0)
+ UNION ALL
+ SELECT level + 1, data
+ FROM r INNER JOIN (VALUES (0, 'x')) AS t(data, tag) USING (data)
+ WHERE level < 2 AND tag = 'x'
+)
+SELECT * FROM r;
+
+-- Recursive CTE self-reference under outer joins with USING and NATURAL:
allowed only on the
Review Comment:
**Nit (P3):** The analyzer also accepts the self-reference on the preserved
side of a right outer join. USING and NATURAL joins are lowered to a plain
`Join` of their base type (`commonNaturalJoinProcessing` in `Analyzer.scala`)
before `CheckAnalysis` runs
`ResolveWithCTE.checkIfSelfReferenceIsPlacedCorrectly`, and that method's
`RightOuter` case forbids the reference only in the left child
(`ResolveWithCTE.scala:341-343`). So `FROM (VALUES (0)) AS t(data) RIGHT OUTER
JOIN r USING (data)` is accepted, the same as the existing ON-join test earlier
in this file ("recursive reference is allowed on right side of a right outer
join"). As written, the comment says right outer joins never allow the
reference, and no case in this block would show otherwise.
Consider rewording it to say the reference is allowed only on the preserved
side of a left or right outer join and is rejected on the null-supplying side
and in full outer joins. Also add a `(VALUES (0)) AS t(data) RIGHT OUTER JOIN r
USING (data)` case so the right-outer USING lowering is pinned, then regenerate
the golden files.
**Verification:**
- **Behavior:** After regeneration, the recursive CTE golden output records
the right outer USING join with the self-reference on the preserved right side
as accepted, returning the same level and data rows as the corresponding left
outer USING case, while the left outer case with the reference on the
null-supplying side and the full outer case remain rejected with
INVALID_RECURSIVE_REFERENCE.PLACE.
- **Inspection:** The reworded comment matches the recursive-reference
placement rule for left, right and full outer joins, including the existing
right-outer ON-join case earlier in the same file.
##########
sql/core/src/test/resources/sql-tests/inputs/window-with-aggregates.sql:
##########
@@ -183,4 +183,120 @@ SELECT
AVG(col1) as avg_col1,
RANK() OVER (PARTITION BY col1 / 5, col2 % 2 ORDER BY AVG(col1)) as
rank_in_bucket
FROM VALUES (5, 1), (7, 3), (10, 2), (12, 4), (15, 1) AS t(col1, col2)
-GROUP BY col1 / 5, col2 % 2, col1, col2;
\ No newline at end of file
+GROUP BY col1 / 5, col2 % 2, col1, col2;
+
+CREATE OR REPLACE TEMPORARY VIEW wwa_data AS SELECT * FROM VALUES
+(1, 10), (1, 20), (2, 30), (3, 40) AS wwa_data(col1, col2);
+
+-- ORDER BY grouping key that is aliased with a different name in SELECT,
+-- combined with a window function (Aggregate + Window + Sort).
+SELECT col1 AS a, count(*) * 1.0 / sum(count(*)) OVER () AS ratio
+FROM wwa_data
+GROUP BY col1
+ORDER BY col1;
+
+-- Same as above but without a window function (Aggregate + Sort).
+SELECT col1 AS a, count(*) AS cnt
+FROM wwa_data
+GROUP BY col1
+ORDER BY col1;
+
+-- HAVING on grouping key that is aliased with a different name in SELECT,
+-- combined with a window function (Aggregate + Window + Filter).
+SELECT col1 AS a, count(*) * 1.0 / sum(count(*)) OVER () AS ratio
+FROM wwa_data
+GROUP BY col1
+HAVING col1 > 0;
+
+-- QUALIFY on grouping key that is aliased with a different name in SELECT,
+-- combined with a window function (Aggregate + Window + Filter).
+SELECT col1 AS a, count(*) * 1.0 / sum(count(*)) OVER () AS ratio
+FROM wwa_data
+GROUP BY col1
+QUALIFY col1 > 0;
+
+-- QUALIFY with a window function in the condition on an aliased grouping key.
+SELECT col1 AS a, count(*) * 1.0 / sum(count(*)) OVER () AS ratio
+FROM wwa_data
+GROUP BY col1
+QUALIFY row_number() OVER (ORDER BY col1) = 1;
+
+-- QUALIFY + ORDER BY on grouping key that is aliased with a different name in
SELECT,
+-- combined with a window function (Aggregate + Window + Filter + Sort).
+-- The Sort child is Filter(_, Window), not Window directly.
+SELECT col1 AS a, count(*) * 1.0 / sum(count(*)) OVER () AS ratio
+FROM wwa_data
+GROUP BY col1
+QUALIFY col1 > 0
+ORDER BY col1;
+
+-- QUALIFY with a window function in the condition + ORDER BY on aliased
grouping key.
+-- The Sort child is Project(_, Filter(_, Window(_, Window(_)))).
+SELECT col1 AS a, count(*) * 1.0 / sum(count(*)) OVER () AS ratio
+FROM wwa_data
+GROUP BY col1
+QUALIFY row_number() OVER (ORDER BY col1) = 1
+ORDER BY col1;
+
+-- GROUPING SETS + window + ORDER BY: GROUPING_ID() aliased by user.
+-- The GroupingAnalyticsTransformer creates a double alias
+-- Alias(Alias(spark_grouping_id, "grouping_id()"), "gid") which must be split
+-- so the Aggregate keeps the internal "grouping_id()" name and the Window
uses "gid".
Review Comment:
**Nit (P3):** The plan this PR records for the query below has no such
split. The Aggregate emits `spark_grouping_id#xL AS gid#xL` directly, no
`grouping_id()`-named attribute appears, and the Window computes only
`sum(_w0#xL)`, never reading `gid`, which passes through the Projects to the
Sort. The double alias itself is real:
`GroupingAnalyticsTransformer.replaceGroupingFunction` wraps the grouping id in
a `grouping_id()` alias inside the user's `gid` alias. But the inner alias is
simply dropped rather than split: it is gone in the fixed-point plan recorded
here, and the single-pass `AliasResolver.collapseAlias` likewise removes the
inner of two stacked aliases (`ResolverGuard` keeps window queries out of the
single-pass dual run anyway). Readers would go looking for an alias-splitting
step that does not exist.
Consider describing what the golden pins instead: the aliased
`GROUPING_ID()` becomes the Aggregate output `gid`, the window aggregate reads
only the per-group count, and `gid` passes through to the ORDER BY.
**Verification:**
- **Inspection:** The reworded comment matches the analyzed plan recorded
for the GROUPING SETS + window + ORDER BY query, in which the Aggregate emits
spark_grouping_id as gid and no grouping_id()-named attribute appears, and the
golden outputs are unchanged by the edit.
##########
sql/core/src/test/resources/sql-tests/inputs/group-by.sql:
##########
@@ -338,3 +338,49 @@ SELECT col1 FROM t1 GROUP BY ALL HAVING any_value(col2) =
'a';
SELECT col1 FROM t1 GROUP BY col1 HAVING EXISTS (SELECT any_value(t1.col2) ==
0);
DROP TABLE t1;
+
+-- Window and generator functions as grouping keys. A window function is
computed after grouping,
+-- so it can never be a grouping key: it is rejected in an explicit GROUP BY,
when GROUP BY ALL
+-- promotes the SELECT window to a grouping key, and when a GROUP BY ordinal
points at it. A
+-- generator is rejected as an explicit grouping key, but a generator in the
SELECT list is
+-- extracted to a Generate operator, so GROUP BY ALL / a GROUP BY ordinal that
points at it groups
+-- by the generated output and stays valid.
Review Comment:
**Nit (P3):** The plans recorded for this block group by the generator's
input, not its output. `ExtractGenerator` moves the generator's arguments into
the Aggregate as `_gen_input_N` aliases and evaluates the generator after
aggregation; the new analyzer golden shows `Generate explode(_gen_input_0#x)`
over `Aggregate [a#x], [a#x AS _gen_input_0#x]` (and `Aggregate [c1#x, a#x]`
for the `c1, explode(a)` variants). Repeated generated values are therefore not
collapsed: `SELECT explode(a) FROM VALUES (array(1, 1)) AS t(a) GROUP BY ALL`
returns `1` twice, where grouping by the generated output would return it once.
None of the test arrays has a repeated element, so `results/group-by.sql.out`
reads the same either way and would not catch a switch between the two
semantics.
Consider rewording it to say the generator is evaluated after aggregation,
so GROUP BY ALL or an ordinal that points at it groups by the generator's
input, and adding a GROUP BY ALL case over an array with a repeated element,
such as `VALUES (array(1, 1))`, then regenerating the golden files.
**Verification:**
- **Behavior:** After regeneration, the GROUP BY ALL generator case over an
array with a repeated element records one output row per generated element (two
rows of 1 for array(1, 1)), consistently across the codegen configurations,
which pins grouping by the generator input.
- **Inspection:** The reworded comment agrees with the recorded analyzed
plans, which group by the generator input with Generate above the Aggregate.
--
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]
