Repository navigation
Conversation
| } | ||
|
|
||
| /** | ||
| * Returns whether an UNNEST call's array operand has a struct element type. |
There was a problem hiding this comment.
beware that unnest has two semantics, depending on conformance: the Trino semantics does not create a column for each ROW column, and the standard semantics does.
There was a problem hiding this comment.
Oh, I was not aware. Thank you for catching that.
|
I think you can squash the commits for merging |
…e when aliased Extend AliasNamespace's CALCITE-5779 COLLECTION_TABLE check to also cover UNNEST when the array's component type is a struct. With allowAliasUnnestItems=true (Trino), UNNEST does not expand struct columns and instead uses the alias directly.
vlsi
left a comment
There was a problem hiding this comment.
The fix makes d.a resolve, but it also changes column names and NATURAL JOIN results for existing queries, leaves two variants of the same bug unfixed, and the test does not guard the half of the contract that keeps the alias usable. Details are in the inline comments.
Commit message and PR description
- The PR title is cut off ("loses the field nam…"), and the description starts with "…e when aliased". Please put the full title into the title field.
- In "Extend AliasNamespace's CALCITE-5779 COLLECTION_TABLE check", the ticket number carries the meaning. Please name the mechanism instead, for example "the single-column alias that keeps the original field name, added in CALCITE-5779".
- "(Trino)" is incomplete:
allowAliasUnnestItemsis true for bothPRESTOandBIG_QUERY. - The commit message should state the user-visible change.
select *over such an UNNEST now returns the field name instead of the alias, andNATURAL JOINmatches on the field name. The JIRA needs the same note so that it reaches the release notes.
|
It looks like some of these objections are legitimate, I will wait until you reply to them. |
- Correct a stale comment claiming the alias always names the sole column - Extracted the || condition into a local isStructUnnest boolean for readability - Address WITH ORDINALITY: select d.a from R with ordinality as d keeps its field name - Address sub-query operand issue: isUnnestOfStructArray now unwraps a struct-typed operand - Corrected the Javadocs
|
@vlsi Thank you for the thorough review. I believe I have addressed all of your concerns. Let me know if you see anything else. |
vlsi
left a comment
There was a problem hiding this comment.
Thanks for the update. d.a now resolves for a plain UNNEST, a sub-query operand, and WITH ORDINALITY, and the PRESTO case has its own test. The remaining problems are below, with details in the inline comments.
In the examples, R = unnest(cast(array[row(1)] as row(a integer) array)). I ran each query on this PR and on its base, 9634752f1a.
Behavior
WITH ORDINALITYnow loses the alias:select d from R with ordinality as dreturned(D)on the base and now fails withColumn 'D' not found in any table. WithoutWITH ORDINALITY,select d from R as dstill works, so the two forms follow different rules.- The reply in the naming thread says the alias is "only usable to qualify" the column, but
select d, d.a from R as dreturns(D, A)in the new test, andselect * from R as d join (values 1) t(d) using (d)is still accepted and returns a column namedA. Please decide whether the alias names the column or only qualifies it, and make the code, the tests, and the comments say the same thing. - MULTISET changes too:
select * from unnest(cast(multiset[row(1)] as row(a integer) multiset)) as dreturned(D)and now returns(A). It has no test.
Commit message and JIRA
- The commit message is unchanged from the first round. It still says "Extend AliasNamespace's CALCITE-5779 COLLECTION_TABLE check" and "(Trino)", and it does not state the user-visible change.
- The PR description has the same first sentence about CALCITE-5779. It also says that the fix holds for
WITH ORDINALITYand that "both the alias and the field name resolve"; with this PR,select d from R with ordinality as dfails, so the second claim is false forWITH ORDINALITY. Please fix the description before reusing it for the commit message. - The commit subject says "a single-field ROW array", and the JIRA summary and the PR title say "a ROW array". Please make them agree; if "single-field" is the accurate scope, the JIRA summary can change.
- The JIRA has no note about the change for the release notes:
select *over such an UNNEST returns the field name instead of the alias, andNATURAL JOINmatches on the field name.
Not found
- I did not find a JIRA for the
CompileException: Cannot cast "Baz$Record1_0" to "java.lang.Object[]"failure that blocks an end-to-end test. If you have filed one, please link it from CALCITE-7791.
|
sbroeder, mihaibudiu, thanks for the ping. I was literally reviewing the PR, so it ended up like "mihaibudiu requested review and vlsi posted a comment 5 minutes later" :) |
SingleColumnAliasRelDataType.java — generalized to support a multi-field original
so it can now dual-name the first field of a struct-array WITH ORDINALITY result too,
not just a single-column UNNEST.
AliasNamespace.java:
- Addressed WITH ORDINALITY again: the data column now uses SingleColumnAliasRelDataType
instead of just returning rowType unchanged. That was silently breaking
select d / USING (d) for the ordinality case
- Reworded and moved comments.
- Renamed isUnnestOfStructArray to keepsElementFieldName
SqlUnnestOperator.java — added another static helper, unwrapOperandType, so the subquery
unwrap step is no longer duplicated between inferReturnType and AliasNamespace. Both new
helpers are now @API(since = "1.43", status = INTERNAL). fixed the Javadoc wording and @params.
SqlValidatorTest.java:
- Fixed comments and wording
- WITH ORDINALITY test now asserts the full select * row type and adds a select d, d.a case.
- Added missing USING and MULTISET cases.
|
@vlsi Thank you for another thorough review. I believe I've addressed your concerns and filed related JIRA https://issues.apache.org/jira/browse/CALCITE-7831. Please let me know if I have inadvertently overlooked something. |
vlsi
left a comment
There was a problem hiding this comment.
Thanks, the inline comments from the last round are addressed, apart from the USING wording below, and thanks for filing CALCITE-7831.
In the examples, R = unnest(cast(array[row(1)] as row(a integer) array)). I ran the queries below on this PR and on its base, 9634752f1a, through the validator and SqlToRelConverter.
select d, d.a from R with ordinality as dreturns(D, A), and the plan projects$0twice.- The sub-query operand with
WITH ORDINALITYresolvesdagain:(D)on the base, an error on the previous headf407a58297, and(D)now. NATURAL JOINandUSING (d)behave the same withWITH ORDINALITYas without it.- A scalar array, a MAP, a ROW with two fields, and PRESTO give the same row types as on the base.
The remaining points:
Behavior
- An alias equal to
ORDINALITYnow resolves where the base failed, and the order of the lookups inSingleColumnAliasRelDataType.getFielddecides which column an unqualified name means. Details are in the inline comment ongetFieldCount, the line that lets the type have more than one field.
Comments and description
- The test comment above the
NATURAL JOINcases says thatUSINGmatches the field name and not the alias, and theUSING (d)case below it shows thatUSINGmatches both. The PR description says the same: "NATURAL JOIN and USING, which now match on the field name rather than the alias".
Commit message and JIRA
- The first commit's message still says "Extend AliasNamespace's CALCITE-5779 COLLECTION_TABLE check" and "(Trino)". The PR description no longer has either, so, once the
USINGsentence is fixed, it can serve as the message when the commits are squashed. - The JIRA still has no note for the release notes:
select *over such an UNNEST returns the field name instead of the alias, andNATURAL JOINmatches on the field name.
| @Override public int getFieldCount() { | ||
| return 1; | ||
| return original.getFieldCount(); | ||
| } |
There was a problem hiding this 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.
There was a problem hiding this comment.
The Javadoc on getField and on the class now states the lookup order. No test pins it yet: on 33a6826 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
SqlToRelConverterTestplan for that query, which projectsORDINALITY=[$1]now and$0with the lookups swapped.testUnnestSingleFieldRowWithOrdinalitycould become this test (see the comment onSqlToRelConverterTest). - 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 ordinalityreturnsRecordType(INTEGER NOT NULL ORDINALITY) NOT NULLnow andVARCHAR(1) NOT NULL ORDINALITYwith the lookups swapped.
| // '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); |
There was a problem hiding this 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 ORDINALITYbranch 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 stillisStructUnnest. The same name for both would be easier to follow.
There was a problem hiding this 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."
| // 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. |
There was a problem hiding this 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.
There was a problem hiding this comment.
This comment is unchanged in 33a6826. The PR description now says that USING accepts either name, and this comment still says the opposite.
| + "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. |
There was a problem hiding this 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."
There was a problem hiding this comment.
This comment is unchanged in 33a6826. The suggestion from the thread still applies: "USING (d) matches the alias, and the output column is named A."
- Update comments to reflect actual behavior and remove ambiguity. - Add new tests in SqlValidatorTest and SqlToRelConverterTest.
|
@vlsi I've posted another revision. Please let me know if you have other requests. |
vlsi
left a comment
There was a problem hiding this comment.
Thanks. The class Javadoc, the getField Javadoc, the new WITH ORDINALITY tests, the PR description, and the release note in the JIRA cover what I asked for in the last round. Four threads are still open; the replies are in those threads:
- The two test comments about
USINGare unchanged. - The query with an alias equal to
ORDINALITYhas no test yet. - The new comment in
AliasNamespacesays "columns" where only one column takes the alias.
| @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(); | ||
| } |
There was a problem hiding this comment.
testUnnestSingleFieldRowWithOrdinalityAllColumnsselectsd, d.a, d.ordinality, which includes everythingtestUnnestSingleFieldRowWithOrdinalityselects, over the sameUncollect(withOrdinality=[true])input.testUnnestSingleFieldRowWithOrdinalityadds nothing; it could become the plan test for the alias equal toORDINALITY(see thegetFieldthread).- The neighboring
testUnnestSingleFieldRowAliasKeepsFieldNamehas the usual "Test case for [CALCITE-7791]" Javadoc; the new tests could have it too.
The comment said 'NATURAL JOIN and USING match the field name' but the USING (d) case below shows USING also accepts the alias. Split into two sentences: NATURAL JOIN matches the field name; USING accepts either name.
|
|
@vlsi Anything else needed on this one? |



When a single-field ROW array is unnested and aliased, the array's
field name was overwritten by the alias instead of being preserved.
The alias now names the column, but the field name is also
preserved and resolves alongside it. This holds for a plain UNNEST,
WITH ORDINALITY, a sub-query UNNEST operand, and MULTISET.
This is visible in
select *, which now includes the field namerather than a column synthesized from the alias. NATURAL JOIN
now matches on the field name rather than the alias, and USING
accepts either name.
Under PRESTO or BIG_QUERY conformance, where UNNEST does not expand a
ROW element into columns, the alias still renames the single ROW
column, unchanged.
The rule for whether UNNEST expands a ROW element into columns, and
the rule for unwrapping a sub-query operand's type, are each defined
once, in SqlUnnestOperator, and shared with AliasNamespace, instead of
being duplicated.
Jira Link
CALCITE-7791