vlsi commented on code in PR #5271:
URL: https://github.com/apache/calcite/pull/5271#discussion_r4097577879
##########
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:
In the examples, `R` = `unnest(cast(array[row(1)] as row(a integer) array))`.
The column now has two names. `SingleColumnAliasRelDataType` exposes the
field as `A` through `getFieldList()`, while `getField("D")` still resolves it
through the alias. Star expansion and `NATURAL JOIN` use the first name;
qualified references and `USING` use the second.
| Query | `main` | This PR |
| --- | --- | --- |
| `select * from R as d` | `(D)` | `(A)` |
| `select * from R as d natural join (select 5 as d) t` | joins on `D`,
`condition=[=($0, $1)]` | cross join, `condition=[true]`, `(A, D)` |
| `select * from R as d natural join (select 5 as a) t` | cross join, `(D,
A)` | joins on `A` |
| `select * from R as d join (values 1) t(d) using (d)` | `(D)` | `USING
(d)` is accepted, and the output column is named `A` |
The `NATURAL JOIN` queries change their result without any error.
CALCITE-4305 (`testUnnestAlias`) established that the alias of a single-column
UNNEST becomes the column name, and this PR changes that for ROW elements.
Please decide which name is canonical and add tests for `NATURAL JOIN` and
`USING`.
##########
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
Review Comment:
The comment above this statement is stale: "then the namespace's sole column
is named after the alias" no longer holds for a single-field ROW element.
##########
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))
Review Comment:
The `||` continuation is indented deeper than the `?`, so the condition and
the branches run together. Consider extracting the condition into a local
`boolean`.
##########
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:
A sub-query operand still has the bug: `select * from unnest((select
cast(array[row(1)] as row(a integer) array) from (values 1))) as d` returns
`(D)`. `SqlUnnestOperator.inferReturnType` unwraps a struct operand to its
first field before it reads the component type. This code calls
`getComponentType()` on the struct itself, gets `null`, and returns `false`.
The rule that decides whether UNNEST expands a ROW element into columns now
lives in two places, and the two already disagree. Please move it into one
static helper in `SqlUnnestOperator` and call it from both.
##########
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:
In the examples, `R` = `unnest(cast(array[row(1)] as row(a integer) array))`.
- The test does not check that the alias still names the column. If
`SingleColumnAliasRelDataType` is replaced with plain `rowType` for this case,
`select d from R as d` breaks and the whole `SqlValidatorTest` stays green.
Please add `sql("select d, d.a from R as d").type("RecordType(INTEGER NOT NULL
D, INTEGER NOT NULL A) NOT NULL")`.
- Some cases have no test: `WITH ORDINALITY`, a sub-query operand, `NATURAL
JOIN`, `USING`, and MULTISET.
- The change also affects the `SqlToRelConverter` output, which becomes
`LogicalProject(D=[$0], A=[$0])` over `Uncollect`, but only the validator is
tested. Please add a `SqlToRelConverterTest` case. An end-to-end test cannot
use this literal: `cast(array[row(1)] as row(a integer) array)` fails on `main`
as well with `CompileException: Cannot cast "Baz$Record1_0" to
"java.lang.Object[]"`. That failure probably deserves its own JIRA.
- `testUnnestAlias` (CALCITE-4305) holds the rule "the alias becomes the
column name" and its exceptions. The new case belongs there, next to the "The
magic doesn't happen if ..." cases.
- The name `testUnnestSingleFieldRow` says what is tested, not the outcome.
Something like `testUnnestSingleFieldRowKeepsFieldName` would read better in a
failure report.
##########
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() {
+ sql("select d.a\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array)) as d")
+ .columnType("INTEGER NOT NULL");
+ // The array's real field name ("A") survives star-expansion too, not just
+ // the synthetic alias ("D").
Review Comment:
`"D"` is the user's alias, not a synthetic one. "not just" suggests that
star expansion returns both names, and it does not.
##########
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() {
+ sql("select d.a\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array)) as d")
+ .columnType("INTEGER NOT NULL");
+ // The array's real field name ("A") survives star-expansion too, not just
+ // the synthetic alias ("D").
+ sql("select *\n"
+ + "from unnest(cast(array[row(1)] as row(a integer) array)) as d")
+ .type("RecordType(INTEGER NOT NULL A) NOT NULL");
+ // Trino: struct not expanded; alias renames the ROW column.
Review Comment:
This comment has no verb. The test uses `PRESTO`, and `BIG_QUERY` also
enables `allowAliasUnnestItems`, so "Trino" names neither.
The `PRESTO` case is a separate behavior. In its own test method, it would
be reported separately when it fails.
##########
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:
In the examples, `R` = `unnest(cast(array[row(1)] as row(a integer) array))`.
`WITH ORDINALITY` still has the bug: `select d.a from R with ordinality as
d` fails with `Column 'A' not found in table 'D'`. This branch renames the
element column to the alias.
##########
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 summary does not match what the method returns. The method also
returns `false` when `allowAliasUnnestItems()` is true or when UNNEST has more
than one operand, and it returns `true` for a MULTISET. The name has the same
problem: under `PRESTO` the operand is an array of structs, and the method
returns `false`.
- "produces a 1-field row for both struct arrays and scalar arrays" holds
only for a single-field struct. A struct with N fields produces N columns, and
a MAP produces two. The condition belongs in the same sentence.
- "worth preserving" does not say what happens to the name. Please state it
directly: the column of a scalar collection is named `EXPR$0`, and the alias
replaces that name.
Suggested wording, if the logic stays here:
```java
/**
* Returns whether the sole column of an aliased UNNEST is a field of the
* collection's ROW element.
*
* <p>Such a column keeps its field name, and the alias names it as well.
* The column of a scalar collection is named {@code EXPR$0}, and the alias
* replaces that name. UNNEST expands a ROW element into one column per field
* only where {@link SqlConformance#allowAliasUnnestItems()} is false.
*/
```
##########
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) {
Review Comment:
This check is unreachable. The caller runs only when the row type has one
field, and every UNNEST operand contributes at least one column. With the check
removed, `SqlValidatorTest` stays green. Please drop it.
--
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]
