[mxtymoshyk]

Fixes #26911

CROSS JOIN UNNEST over an ARRAY<ROW<...>> whose element has a nested ROW field fails when the query runs through the JDBC driver or the SQL shell:

java.lang.IllegalArgumentException: Row expected 4 fields (Field{name=ref, ...}, Field{name=commits, ...}, Field{name=id, ...}, Field{name=author, type=ROW<name STRING, email STRING>, ...}). initialized with 5 fields.
  at org.apache.beam.sdk.values.Row$Builder.build(Row.java:861)
  at org.apache.beam.sdk.extensions.sql.impl.rel.BeamUnnestRel$UnnestFn.process(BeamUnnestRel.java:195)

Beam plans SQL with two different Calcite planners, and they give different unnest output for the same query. The planner behind SqlTransform flattens the element's nested row into leaf columns (id, name, email). The planner behind the JDBC driver keeps it as one ROW column (id, author). UnnestFn always flattened, so it only matched SqlTransform.

UnnestFn now checks each nested row field of the element against the output column at the same position. If that column is a ROW, the row goes in as is. Otherwise the row's values go in flattened.

The same change fixes a null nested row on the flattened path. The old code added no values for it, so the row came out short and hit the same field count error. It now fills one null per flattened column.

Tests:

• JdbcDriverTest#testUnnestArrayOfRowsWithNestedRow covers the JDBC path, including an element whose nested row is null.
• BeamSqlDslUnnestRowsTest#testUnnestArrayWithNullNestedRow covers a null nested row on the SqlTransform path.
• Both fail without the change to BeamUnnestRel. The existing testUnnestArrayWithNestedRows still passes and covers the flattened path.
• ./gradlew :sdks:java:extensions:sql:check passes locally.

[github-actions]

📋 Review Summary

This PR fixes an IllegalArgumentException in BeamUnnestRel.UnnestFn when running CROSS JOIN UNNEST over an ARRAY<ROW<...>> containing nested ROW fields via the JDBC driver or SQL shell, as well as when flattening null nested rows via SqlTransform. The implementation cleanly aligns UnnestFn value extraction with outputSchema and includes unit tests covering both the JDBC and SqlTransform paths.

🔍 General Feedback

• Checking the corresponding outputSchema column type in addElementValues accurately handles both the unflattened (JdbcDriver) and flattened (SqlTransform) Calcite planner outputs, while properly padding null values for flattened null nested rows.

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

[utafrali]

The positional-check approach in addElementValues is a clean and correct way to handle both planner outputs without duplicating logic. The fix is well-documented in the Javadoc and the two new tests cover the cases that previously crashed. The comments above are minor polish items that do not block merging.

[utafrali]

Objects.nonNull(element) is equivalent to element != null but less idiomatic in Java. Prefer the direct null check: if (element != null). The rest of the file uses direct null comparisons (e.g. rawValues == null on line 184).

[mxtymoshyk]

Done in 390bfad. Switched to element != null to match the rest of the module.

[utafrali]

isRowColumn will throw IndexOutOfBoundsException if index >= outputSchema.getFieldCount(). This can only happen when the element schema and the output schema disagree on field count, which indicates a planning bug rather than user error. A bounds guard with a clear message would make that failure much easier to diagnose than a raw IOOBE:

private boolean isRowColumn(int index) {
  if (index >= outputSchema.getFieldCount()) {
    throw new IllegalStateException(
        "Element schema produces more fields than output schema at index " + index);
  }
  return outputSchema.getField(index).getType().getTypeName().equals(Schema.TypeName.ROW);
}

[mxtymoshyk]

Added a guard in 7a2a631. Instead of throwing, isRowColumn returns false past the last output column. The extra values then reach Row.Builder#build(), which fails with the same Row expected N fields ... initialized with M fields error as in #26911. That message lists the output schema fields, so a planner mismatch is easier to trace. Happy to switch to an IllegalStateException if you would rather have that.

[utafrali]

The new JDBC test only exercises the non-null nested row path. The null-nested-row fix in addElementValues (adding a null per flattened column) is covered by BeamSqlDslUnnestRowsTest#testUnnestArrayWithNullNestedRow, but that test runs through SqlTransform, not the JDBC planner. On the JDBC path the row is kept as a single ROW column, so a null nestedRow would be added as a single null value via values.add(value) at line 160, which is a different branch. Consider adding a second entry in tableProvider.addRows with a null nested row and asserting that the corresponding result row has null for the nested field.

[mxtymoshyk]

Added in 7a2a631. The JDBC test now has a third element whose nestedRow is null, and the query returns null for item.nestedRow.name on that row.

[codecov]

Codecov Report

❌ Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.17%. Comparing base (2213e54) to head (390bfad).
⚠️ Report is 34 commits behind head on master.

Files with missing lines	Patch %	Lines
...eam/sdk/extensions/sql/impl/rel/BeamUnnestRel.java	90.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #40841       +/-   ##
=============================================
+ Coverage          0   64.17%   +64.17%     
- Complexity        0     2357     +2357     
=============================================
  Files             0      321      +321     
  Lines             0    15599    +15599     
  Branches          0     2030     +2030     
=============================================
+ Hits              0    10011    +10011     
- Misses            0     4822     +4822     
- Partials          0      766      +766     

Flag	Coverage Δ
java	64.17% <90.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Assigning reviewers:

R: @chamikaramj for label java.
Backup reviewer: Abacn (if the main reviewer is unresponsive, you can escalate to them by tagging their username)

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

• stop reviewer notifications - opt out of the automated review tooling
• remind me after tests pass - tag the comment author after tests pass
• waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).