vlsi commented on code in PR #5271:
URL: https://github.com/apache/calcite/pull/5271#discussion_r4136654657
##########
core/src/main/java/org/apache/calcite/sql/validate/AliasNamespace.java:
##########
@@ -111,6 +111,7 @@ protected AliasNamespace(
rowType.getFieldList().get(0).getType())
.build();
aliasedType = node.getKind() == SqlKind.COLLECTION_TABLE
+ || (node.getKind() == SqlKind.UNNEST &&
isUnnestOfStructArray((SqlCall) node))
? new SingleColumnAliasRelDataType(rowType, singleColumnAlias) :
singleColumnAlias;
// If the sub-query is UNNEST with ordinality
Review Comment:
`d.a` resolves now, but the alias no longer names the data column:
| Query | Base | This PR |
| --- | --- | --- |
| `select d from R with ordinality as d` | `(D)` | `Column 'D' not found in
any table` |
| `select * from R with ordinality as d` | `(D, ORDINALITY)` | `(A,
ORDINALITY)` |
| `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` |
For a plain UNNEST, `SingleColumnAliasRelDataType` keeps both names, and
this branch returns `rowType`, so only the field name resolves. Please make the
`WITH ORDINALITY` branch follow the same rule as the single-column branch, and
add `select d` and `select *` cases to the test. `SingleColumnAliasRelDataType`
cannot be reused as is: its constructor asserts that both types have one field,
and `getFieldCount()` returns 1, so the two-column case needs that class
generalized or a separate one.
##########
core/src/main/java/org/apache/calcite/sql/validate/AliasNamespace.java:
##########
@@ -100,29 +100,30 @@ 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's a
+ // struct-array UNNEST field (isUnnestOfStructArray), which keeps its
name.
Review Comment:
- "struct-array UNNEST field" is a term this comment invents. Something like
"a field of the collection's ROW element" says the same with words the rest of
the code uses.
- The method name in parentheses adds nothing the next line does not show.
- The comment leaves out `COLLECTION_TABLE`, which also keeps its column
name through `SingleColumnAliasRelDataType`.
##########
core/src/main/java/org/apache/calcite/sql/validate/AliasNamespace.java:
##########
@@ -100,29 +100,30 @@ 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's a
+ // struct-array UNNEST field (isUnnestOfStructArray), which keeps its
name.
+ final boolean isStructUnnest = node.getKind() == SqlKind.UNNEST
+ && isUnnestOfStructArray((SqlCall) node);
if (rowType.getFieldCount() == 1) {
final RelDataType singleColumnAlias =
validator.getTypeFactory().builder()
.kind(rowType.getStructKind())
.add(((SqlIdentifier) operands.get(1)).getSimple(),
rowType.getFieldList().get(0).getType())
.build();
- aliasedType = node.getKind() == SqlKind.COLLECTION_TABLE
+ aliasedType = node.getKind() == SqlKind.COLLECTION_TABLE ||
isStructUnnest
? new SingleColumnAliasRelDataType(rowType, singleColumnAlias) :
singleColumnAlias;
- // If the sub-query is UNNEST with ordinality
- // and the sub-query has two columns: data column, ordinality column
- // then the namespace's sole column is named after the alias.
+ // WITH ORDINALITY: data column named after the alias, unless it's
+ // a struct-array field, which keeps its name.
Review Comment:
This comment sits after the assignment of the single-column branch and
before `} else if`, so it reads as part of the previous branch. Please move it
into the `WITH ORDINALITY` branch. It also says only that the field keeps its
name; with this branch returning `rowType`, the alias no longer names the
column (see the [WITH ORDINALITY
thread](https://github.com/apache/calcite/pull/5271#discussion_r4097577914)).
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -10101,6 +10101,55 @@ 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 array's field name ("A") survives
+ // star-expansion, and the alias ("D") still works to qualify it.
+ 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");
Review Comment:
"the alias ("D") still works to qualify it" describes `d.a`, but the query
also selects `d` as a column, and the expected type has a column `D`. Please
say both: the alias names the column, and it qualifies the field. (If you
decide that the alias should only qualify, this assertion changes too; see the
[canonical-name
thread](https://github.com/apache/calcite/pull/5271#discussion_r4097577879).)
##########
core/src/main/java/org/apache/calcite/sql/SqlUnnestOperator.java:
##########
@@ -130,6 +127,15 @@ public SqlUnnestOperator(boolean withOrdinality) {
return builder.build();
}
+ /**
+ * Returns whether UNNEST expands {@code componentType} into one column
+ * per field, rather than a single column of the struct's own type.
+ */
+ public static boolean expandsStructIntoColumns(RelDataType componentType,
+ boolean allowAliasUnnestItems) {
+ return componentType.isStruct() && !allowAliasUnnestItems;
+ }
Review Comment:
- This is a new `public` method, public only because `AliasNamespace` is in
another package. Consider `@API(since = "1.43", status = API.Status.INTERNAL)`.
- `allowAliasUnnestItems` has no `@param`. The inline comment this PR
removed from `inferReturnType` said that the expansion depends on the SQL
conformance; the `@param` is the place for that fact.
- "rather than a single column of the struct's own type" describes only a
struct. For a scalar component type the method returns `false`, and there is no
struct column. Suggested wording:
```java
/**
* Returns whether UNNEST expands an element of type {@code componentType}
* into one column per field.
*
* @param componentType element type of the collection
* @param allowAliasUnnestItems value of
* {@link SqlConformance#allowAliasUnnestItems()}; when true, a ROW
* element becomes a single column of the ROW type
*/
```
The `{@link}` needs an import of
`org.apache.calcite.sql.validate.SqlConformance`; checkstyle accepts an import
used only in Javadoc.
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -10101,6 +10101,55 @@ 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 array's field name ("A") survives
+ // star-expansion, and the alias ("D") still works to qualify it.
+ 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");
+
+ // UNNEST ROW array in a sub-query.
+ 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.
+ sql("select d.a\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array))\n"
+ + " with ordinality as d")
+ .columnType("INTEGER NOT NULL");
+
+ // NATURAL JOIN and USING match 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");
+ }
+
+ /** 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>.
+ *
+ * <p>Under {@link SqlConformanceEnum#PRESTO} or {@code BIG_QUERY}, the
Review Comment:
Nit: `{@link SqlConformanceEnum#PRESTO}` next to `{@code BIG_QUERY}`.
`{@link SqlConformanceEnum#BIG_QUERY}` would be consistent.
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -10101,6 +10101,55 @@ 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 array's field name ("A") survives
+ // star-expansion, and the alias ("D") still works to qualify it.
+ 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");
+
+ // UNNEST ROW array in a sub-query.
Review Comment:
"UNNEST ROW array in a sub-query." has no verb and no outcome. For example:
"A sub-query operand keeps the field name as well."
##########
core/src/main/java/org/apache/calcite/sql/validate/AliasNamespace.java:
##########
@@ -111,6 +111,7 @@ protected AliasNamespace(
rowType.getFieldList().get(0).getType())
.build();
aliasedType = node.getKind() == SqlKind.COLLECTION_TABLE
+ || (node.getKind() == SqlKind.UNNEST &&
isUnnestOfStructArray((SqlCall) node))
? new SingleColumnAliasRelDataType(rowType, singleColumnAlias) :
singleColumnAlias;
Review Comment:
> The field name (A) is canonical; the alias (D) is only usable to qualify
it.
The code does more than that. `select d, d.a from R as d` returns `(D, A)`,
so `d` also resolves as a column, and the new test asserts it. `join (values 1)
t(d) using (d)` is still accepted, and its output column is named `A`. If the
alias should only qualify the column, both queries should fail; if it should
also name the column, the reply and the test comment need to say so. Either
way, please add the `USING (d)` query to the tests.
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -10101,6 +10101,55 @@ 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 array's field name ("A") survives
+ // star-expansion, and the alias ("D") still works to qualify it.
+ 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");
+
+ // UNNEST ROW array in a sub-query.
+ 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.
+ sql("select d.a\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array))\n"
+ + " with ordinality as d")
+ .columnType("INTEGER NOT NULL");
Review Comment:
- The comment has a double space and no verb.
- `columnType` checks only that `d.a` resolves. It passes with the
regression described in the [WITH ORDINALITY
thread](https://github.com/apache/calcite/pull/5271#discussion_r4097577914).
Please assert the full row type of `select *` and add `select d`.
##########
core/src/main/java/org/apache/calcite/sql/validate/AliasNamespace.java:
##########
@@ -162,6 +163,25 @@ protected AliasNamespace(
}
}
+ /**
+ * Returns whether an UNNEST call's array operand has a struct element type.
+ *
+ * <p>{@link SqlUnnestOperator#inferReturnType} produces a 1-field row for
+ * both struct arrays and scalar arrays. Only the structs have a real field
+ * name worth preserving.
+ */
+ private boolean isUnnestOfStructArray(SqlCall unnestCall) {
+ if (unnestCall.operandCount() != 1) {
+ return false;
+ }
+ if (validator.config().conformance().allowAliasUnnestItems()) {
+ return false;
+ }
+ final RelDataType operandType =
validator.getValidatedNodeType(unnestCall.operand(0));
+ final RelDataType componentType = operandType.getComponentType();
+ return componentType != null && componentType.isStruct();
Review Comment:
`SqlUnnestOperator.expandsStructIntoColumns` covers only the last step,
`isStruct() && !allowAliasUnnestItems`. The step that caused the sub-query bug,
taking the first field of a struct-typed operand, is still written twice: in
`inferReturnType` and in `isUnnestOfStructArray`, whose comment says "unwrap as
SqlUnnestOperator#inferReturnType does". Please move the unwrapping into the
helper too, so that it takes the operand type and both callers pass the operand
type.
##########
core/src/main/java/org/apache/calcite/sql/validate/AliasNamespace.java:
##########
@@ -162,6 +163,25 @@ protected AliasNamespace(
}
}
+ /**
+ * Returns whether an UNNEST call's array operand has a struct element type.
+ *
+ * <p>{@link SqlUnnestOperator#inferReturnType} produces a 1-field row for
+ * both struct arrays and scalar arrays. Only the structs have a real field
+ * name worth preserving.
Review Comment:
The Javadoc is better, but the name is unchanged. `isUnnestOfStructArray`
returns `false` for an array of structs under `PRESTO` and `true` for a
MULTISET, so the name still describes the operand and not the result. The
Javadoc says "the sole column", and the method is also used in the `WITH
ORDINALITY` branch, where there are two columns. A name that states the result,
for example `keepsElementFieldName`, would fit both callers.
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -1201,6 +1201,25 @@ void testDyadicCollateOperator() {
});
}
+ /** 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 testUnnestSingleFieldRow() {
Review Comment:
The list of cases does not include MULTISET, and the behavior changes there
as well: `select * from unnest(cast(multiset[row(1)] as row(a integer)
multiset)) as d` returned `(D)` on the base and returns `(A)` with this PR.
Please add it next to the other CALCITE-7791 cases in `testUnnestAlias`.
--
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]
