vlsi commented on code in PR #5271:
URL: https://github.com/apache/calcite/pull/5271#discussion_r4149677928
##########
core/src/main/java/org/apache/calcite/rel/type/SingleColumnAliasRelDataType.java:
##########
@@ -59,7 +63,7 @@ public SingleColumnAliasRelDataType(RelDataType original,
RelDataType alias) {
}
@Override public int getFieldCount() {
- return 1;
+ return original.getFieldCount();
}
Review Comment:
`getField` looks up a name in `original` first and in the alias second. With
a single-field `original`, both lookups find the same column, so the order did
not matter. With `WITH ORDINALITY`, `original` has a second field, and a name
that matches it wins over the alias:
| Query | Base | This PR |
| --- | --- | --- |
| `select * from R with ordinality as ordinality` | `Column 'ORDINALITY' is
ambiguous` | `(A, ORDINALITY)` |
| `select ordinality from R with ordinality as ordinality` | `Column
'ORDINALITY' is ambiguous` | `(ORDINALITY)`, the plan projects `$1` |
| `select ordinality.a from R with ordinality as ordinality` | `Column 'A'
not found in table 'ORDINALITY'` | `(A)` |
The base failed because its row type had two fields named `ORDINALITY`, so
the new results look like an improvement. What no test pins is that the
unqualified `ordinality` means the ordinality column and not the data column:
swapping the two lookups here leaves the suite green. Please add the second
query to `testUnnestAlias`, and state the lookup order in the Javadoc: a name
that matches a field of the wrapped type resolves to that field, and the alias
applies only to a name that matches none.
##########
core/src/main/java/org/apache/calcite/rel/type/SingleColumnAliasRelDataType.java:
##########
@@ -27,20 +27,24 @@
import java.util.List;
/**
- * Specific type of RelDataType that corresponds to a single column table,
- * where column can have alias.
+ * Specific type of RelDataType where the first column can also be
+ * referenced by an alias.
*
* <p>For instance:
* <blockquote><pre>select rmp, rmp.i from table(ramp(3)) as
rmp;</pre></blockquote>
*
+ * <p>{@code original} may have more than one field, for example a
+ * struct-array UNNEST WITH ORDINALITY, whose second field is ORDINALITY;
+ * only the first field gets the extra alias name.
Review Comment:
- "struct-array UNNEST" is the term that was replaced in `AliasNamespace` in
this round. "UNNEST WITH ORDINALITY of a collection of single-field ROW values"
says the same with words the code uses.
- `{@code original}` refers to a constructor parameter the class comment
does not introduce. "The wrapped type may have more than one field" reads
without the constructor.
- The paragraph says which field gets the alias, but not which name wins
when the alias equals the name of another field (see the comment on
`getFieldCount`).
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -10101,6 +10101,72 @@ void testGroupExpressionEquivalenceParams() {
sql("select * from (SELECT ARRAY['banana'] as fruits) as t,
UNNEST(t.fruits)")
.type("RecordType(CHAR(6) NOT NULL ARRAY NOT NULL FRUITS, "
+ "CHAR(6) NOT NULL EXPR$0) NOT NULL").ok();
+
+ // Test case for [CALCITE-7791] UNNEST of a single-field ROW array loses
+ // the field name when aliased. The field name "A" survives
+ // star-expansion. The alias "D" still names the column and
+ // qualifies the field.
+ sql("select d, d.a\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array)) as d")
+ .type("RecordType(INTEGER NOT NULL D, INTEGER NOT NULL A) NOT NULL");
+
+ // A sub-query operand keeps the field name as well.
+ sql("select *\n"
+ + "from unnest(\n"
+ + " (select cast(array[row(1)] as row(a integer) array)\n"
+ + " from (values (1)))) as d")
+ .type("RecordType(INTEGER NOT NULL A) NOT NULL");
+
+ // WITH ORDINALITY keeps the field name and the alias still
+ // names the data column.
+ sql("select *\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array))\n"
+ + " with ordinality as d")
+ .type("RecordType(INTEGER NOT NULL A, INTEGER NOT NULL ORDINALITY) NOT
NULL");
+ sql("select d, d.a\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array))\n"
+ + " with ordinality as d")
+ .type("RecordType(INTEGER NOT NULL D, INTEGER NOT NULL A) NOT NULL");
Review Comment:
A few `WITH ORDINALITY` queries that this round fixed have no test:
| Query | Base | `f407a58297` | This PR |
| --- | --- | --- | --- |
| `select d from unnest((select cast(array[row(1)] as row(a integer) array)
from (values (1)))) with ordinality as d` | `(D)` | `Column 'D' not found in
any table` | `(D)` |
| `select * from R with ordinality as d join (values 1) t(d) using (d)` |
`(D, ORDINALITY)` | `Column 'D' not found in any table` | `(A, ORDINALITY)` |
| a correlated `lateral unnest(t.x) with ordinality as d` that selects `d,
d.a, d.ordinality` | `Column 'A' not found in table 'D'` | `Column 'D' not
found in any table` | resolves, `LogicalCorrelate` in the plan |
The sub-query row is the third row of the table in the [WITH ORDINALITY
thread](https://github.com/apache/calcite/pull/5271#discussion_r4136654657).
Please add at least that one and the `USING (d)` one next to these two cases.
`SqlToRelConverterTest` pins the plan only for a plain UNNEST; a `WITH
ORDINALITY` case there would show `LogicalProject(D=[$0], A=[$0],
ORDINALITY=[$1])`.
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -10101,6 +10101,72 @@ void testGroupExpressionEquivalenceParams() {
sql("select * from (SELECT ARRAY['banana'] as fruits) as t,
UNNEST(t.fruits)")
.type("RecordType(CHAR(6) NOT NULL ARRAY NOT NULL FRUITS, "
+ "CHAR(6) NOT NULL EXPR$0) NOT NULL").ok();
+
+ // Test case for [CALCITE-7791] UNNEST of a single-field ROW array loses
+ // the field name when aliased. The field name "A" survives
+ // star-expansion. The alias "D" still names the column and
+ // qualifies the field.
+ sql("select d, d.a\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array)) as d")
+ .type("RecordType(INTEGER NOT NULL D, INTEGER NOT NULL A) NOT NULL");
+
+ // A sub-query operand keeps the field name as well.
+ sql("select *\n"
+ + "from unnest(\n"
+ + " (select cast(array[row(1)] as row(a integer) array)\n"
+ + " from (values (1)))) as d")
+ .type("RecordType(INTEGER NOT NULL A) NOT NULL");
+
+ // WITH ORDINALITY keeps the field name and the alias still
+ // names the data column.
+ sql("select *\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array))\n"
+ + " with ordinality as d")
+ .type("RecordType(INTEGER NOT NULL A, INTEGER NOT NULL ORDINALITY) NOT
NULL");
+ sql("select d, d.a\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array))\n"
+ + " with ordinality as d")
+ .type("RecordType(INTEGER NOT NULL D, INTEGER NOT NULL A) NOT NULL");
+
+ // UNNEST MULTISET gets the same treatment.
+ sql("select *\n"
+ + "from unnest(cast(multiset[row(1)] as row(a integer) multiset)) as
d")
+ .type("RecordType(INTEGER NOT NULL A) NOT NULL");
+
+ // NATURAL JOIN and USING match the field name "A", not the alias "D".
+ // A column named after the alias does not count as a match.
Review Comment:
"NATURAL JOIN and USING match the field name "A", not the alias "D"" holds
for `NATURAL JOIN` only. The `USING (d)` case at the end of this group shows
that `USING` accepts the alias too. For example: "NATURAL JOIN matches the
field name "A", not the alias "D". USING accepts either name.", keeping the
next line as it is.
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -10101,6 +10101,72 @@ void testGroupExpressionEquivalenceParams() {
sql("select * from (SELECT ARRAY['banana'] as fruits) as t,
UNNEST(t.fruits)")
.type("RecordType(CHAR(6) NOT NULL ARRAY NOT NULL FRUITS, "
+ "CHAR(6) NOT NULL EXPR$0) NOT NULL").ok();
+
+ // Test case for [CALCITE-7791] UNNEST of a single-field ROW array loses
+ // the field name when aliased. The field name "A" survives
+ // star-expansion. The alias "D" still names the column and
+ // qualifies the field.
+ sql("select d, d.a\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array)) as d")
+ .type("RecordType(INTEGER NOT NULL D, INTEGER NOT NULL A) NOT NULL");
+
+ // A sub-query operand keeps the field name as well.
+ sql("select *\n"
+ + "from unnest(\n"
+ + " (select cast(array[row(1)] as row(a integer) array)\n"
+ + " from (values (1)))) as d")
+ .type("RecordType(INTEGER NOT NULL A) NOT NULL");
+
+ // WITH ORDINALITY keeps the field name and the alias still
+ // names the data column.
+ sql("select *\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array))\n"
+ + " with ordinality as d")
+ .type("RecordType(INTEGER NOT NULL A, INTEGER NOT NULL ORDINALITY) NOT
NULL");
+ sql("select d, d.a\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array))\n"
+ + " with ordinality as d")
+ .type("RecordType(INTEGER NOT NULL D, INTEGER NOT NULL A) NOT NULL");
+
+ // UNNEST MULTISET gets the same treatment.
+ sql("select *\n"
+ + "from unnest(cast(multiset[row(1)] as row(a integer) multiset)) as
d")
+ .type("RecordType(INTEGER NOT NULL A) NOT NULL");
+
+ // NATURAL JOIN and USING match the field name "A", not the alias "D".
+ // A column named after the alias does not count as a match.
+ sql("select *\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array)) as d\n"
+ + "natural join (select 1 as a) as t")
+ .type("RecordType(INTEGER NOT NULL A) NOT NULL");
+ sql("select *\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array)) as d\n"
+ + "natural join (select 1 as d) as t")
+ .type("RecordType(INTEGER NOT NULL A, INTEGER NOT NULL D) NOT NULL");
+ sql("select *\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array)) as d\n"
+ + "join (values (1)) as t (a) using (a)")
+ .type("RecordType(INTEGER NOT NULL A) NOT NULL");
+ // USING also accepts the alias, which resolves through the field.
Review Comment:
"which resolves through the field" does not say what the reader should
check. The assertion shows the outcome, so the comment can state it: "USING (d)
matches the alias, and the output column is named A."
##########
core/src/main/java/org/apache/calcite/sql/validate/AliasNamespace.java:
##########
@@ -100,29 +100,36 @@ protected AliasNamespace(
final RelDataType aliasedType;
if (operands.size() == 2) {
final SqlNode node = operands.get(0);
- // Alias is 'AS t' (no column list).
- // If the sub-query is UNNEST or VALUES,
- // and the sub-query has one column,
- // then the namespace's sole column is named after the alias.
+ // 'AS t': the sole column is named after the alias, unless it is a
+ // field of the collection's ROW element. COLLECTION_TABLE's sole
+ // column keeps its own name the same way.
+ final boolean isStructUnnest = node.getKind() == SqlKind.UNNEST
+ && keepsElementFieldName((SqlCall) node);
Review Comment:
- "the sole column is named after the alias, unless it is a field of the
collection's ROW element" says that such a field does not get the alias. It
gets both names: the field keeps its own name for star expansion, and the alias
names it as well.
- The comment now covers the `WITH ORDINALITY` branch too, where there are
two columns, so "the sole column" is too narrow.
- The method is now `keepsElementFieldName`, and the variable that stores
its result is still `isStructUnnest`. The same name for both would be easier to
follow.
--
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]
