Skip to content

Keep the expanded column addressable in mvexpand - #5810

Draft
mch2 wants to merge 1 commit into
opensearch-project:mainfrom
mch2:mvexpand-record-elements
Draft

mch2 wants to merge 1 commit into
opensearch-project:mainfrom
mch2:mvexpand-record-elements

Conversation

@mch2

@mch2 mch2 commented Sep 25, 2026

Copy link
Copy Markdown
Member

mvexpand over an array whose elements are records lost the column it expanded. Uncollect explodes a record element into one column per record field, so the row type becomes the element's field names and the field(arrayFieldName) lookup right after it cannot find the array's own name: mvexpand EVENTS over ARRAY<ROW<NAME, TIME>> fails with "field [EVENTS] not found; input fields are: [ID, TAGS, NAME, TIME]".

The exploded columns are re-wrapped into a single ROW column named after the array, so the expanded column keeps its name and a leaf stays addressable as <array>.<leaf> downstream. The projection is forced, because it is an identity on field references and RelBuilder would otherwise elide it and drop the rewrap.

An element that is not a record needs nothing: ARRAY, and the ARRAY a nested mapping declares, already yield exactly one column carrying the array's name. So this only adds a case, leaving every shape that works today on its existing path.

Tests pin both halves against a table with an ARRAY column and an ARRAY column: the record array gains the ROW($0, $1) rewrap above Uncollect and fails without it, and the scalar array plans exactly as before, with no rewrap.

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

`mvexpand` over an array whose elements are records lost the column it
expanded. Uncollect explodes a record element into one column per record
field, so the row type becomes the element's field names and the
`field(arrayFieldName)` lookup right after it cannot find the array's own
name: `mvexpand EVENTS` over ARRAY<ROW<NAME, TIME>> fails with
"field [EVENTS] not found; input fields are: [ID, TAGS, NAME, TIME]".

The exploded columns are re-wrapped into a single ROW column named after the
array, so the expanded column keeps its name and a leaf stays addressable as
`<array>.<leaf>` downstream. The projection is forced, because it is an
identity on field references and RelBuilder would otherwise elide it and
drop the rewrap.

An element that is not a record needs nothing: ARRAY<scalar>, and the
ARRAY<ANY> a `nested` mapping declares, already yield exactly one column
carrying the array's name. So this only adds a case, leaving every shape
that works today on its existing path.

Tests pin both halves against a table with an ARRAY<ROW> column and an
ARRAY<VARCHAR> column: the record array gains the `ROW($0, $1)` rewrap above
Uncollect and fails without it, and the scalar array plans exactly as before,
with no rewrap.

Signed-off-by: Marc Handalian <handalm@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add null check for array type

Add a null check for arrayFieldRex.getType() before calling getComponentType(). If
the array field type is null, this could result in a NullPointerException. Consider
handling this edge case explicitly.

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [5131-5132]

-RelDataType elementType = arrayFieldRex.getType().getComponentType();
-if (elementType != null && elementType.isStruct()) {
+RelDataType arrayType = arrayFieldRex.getType();
+if (arrayType != null) {
+  RelDataType elementType = arrayType.getComponentType();
+  if (elementType != null && elementType.isStruct()) {
Suggestion importance[1-10]: 3

__

Why: While adding a null check for arrayFieldRex.getType() is a defensive programming practice, the suggestion assumes an edge case that may not occur in the actual context. The arrayFieldRex is derived from correlArrayFieldAccess which is constructed earlier in the method, and its type should be guaranteed to be non-null by the Calcite framework. The suggestion adds defensive code but may not address a realistic issue in this context.

Low

@codecov

codecov Bot commented Sep 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.22%. Comparing base (3f7048b) to head (16e27ba).

Files with missing lines Patch % Lines
.../opensearch/sql/calcite/CalciteRelNodeVisitor.java 0.00% 6 Missing ⚠️

❌ Your project check has failed because the head coverage (63.22%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5810      +/-   ##
============================================
- Coverage     63.23%   63.22%   -0.01%     
  Complexity     8823     8823              
============================================
  Files           938      938              
  Lines         40236    40242       +6     
  Branches       4537     4538       +1     
============================================
  Hits          25445    25445              
- Misses        13966    13972       +6     
  Partials        825      825              
Flag Coverage Δ
sql-engine 63.22% <0.00%> (-0.01%) ⬇️

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant