vlsi commented on code in PR #5271:
URL: https://github.com/apache/calcite/pull/5271#discussion_r4174190637
##########
core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java:
##########
@@ -2314,6 +2314,31 @@ private void checkCallTypesConsistent(String sql) {
sql(sql).ok();
}
+ /**
+ * Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7791";>[CALCITE-7791]
+ * UNNEST of a single-field ROW array loses the field name when aliased</a>.
+ */
+ @Test void testUnnestSingleFieldRowAliasKeepsFieldName() {
+ final String sql = "select d, d.a\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array)) as d";
+ sql(sql).ok();
+ }
+
+ @Test void testUnnestSingleFieldRowWithOrdinality() {
+ final String sql = "select d, d.a\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array))\n"
+ + " with ordinality as d";
+ sql(sql).ok();
+ }
+
+ @Test void testUnnestSingleFieldRowWithOrdinalityAllColumns() {
+ final String sql = "select d, d.a, d.ordinality\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array))\n"
+ + " with ordinality as d";
+ sql(sql).ok();
+ }
Review Comment:
- `testUnnestSingleFieldRowWithOrdinalityAllColumns` selects `d, d.a,
d.ordinality`, which includes everything
`testUnnestSingleFieldRowWithOrdinality` selects, over the same
`Uncollect(withOrdinality=[true])` input.
`testUnnestSingleFieldRowWithOrdinality` adds nothing; it could become the plan
test for the alias equal to `ORDINALITY` (see the `getField` thread).
- The neighboring `testUnnestSingleFieldRowAliasKeepsFieldName` has the
usual "Test case for [CALCITE-7791]" Javadoc; the new tests could have it too.
##########
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:
The Javadoc on `getField` and on the class now states the lookup order. No
test pins it yet: on 33a68262b7 I swapped the two lookups in `getField`, and
`SqlValidatorTest` and `SqlToRelConverterTest` both stay green.
My suggestion last time to add `select ordinality from R with ordinality as
ordinality` to `testUnnestAlias` would not catch the swap either: with `row(a
integer)`, the data column and the ordinality column are both `INTEGER NOT
NULL`, so the row type is the same whichever lookup wins. Either of these does
catch it:
- A `SqlToRelConverterTest` plan for that query, which projects
`ORDINALITY=[$1]` now and `$0` with the lookups swapped.
`testUnnestSingleFieldRowWithOrdinality` could become this test (see the
comment on `SqlToRelConverterTest`).
- A validator case with a field of another type: `select ordinality from
unnest(cast(array[row('x')] as row(a varchar(1)) array)) with ordinality as
ordinality` returns `RecordType(INTEGER NOT NULL ORDINALITY) NOT NULL` now and
`VARCHAR(1) NOT NULL ORDINALITY` with the lookups swapped.
##########
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 variable name and the description of the ROW element's field are fixed.
"columns are named after the alias" is now too broad: only the data column
takes the alias, and the `ORDINALITY` column keeps its name. With a two-field
ROW or a MAP, `WITH ORDINALITY` gives `(A, B, ORDINALITY)` or `(KEY, VALUE,
ORDINALITY)`, and the alias names no column. For example: "'AS t' on a
single-column result, or on UNNEST WITH ORDINALITY with one data column: the
data column is named after the alias. A ROW element's field keeps its own name
for star expansion, and the alias names it as well. COLLECTION_TABLE handles
its column the same way."
##########
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:
This comment is unchanged in 33a68262b7. The PR description now says that
USING accepts either name, and this comment still says the opposite.
##########
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:
This comment is unchanged in 33a68262b7. The suggestion from the thread
still applies: "USING (d) matches the alias, and the output column is named A."
--
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]
