Skip to content

Map Calcite ROW to STRUCT in convertSqlTypeNameToExprType - #5737

Open
mch2 wants to merge 1 commit into
opensearch-project:mainfrom
mch2:row-to-struct-exprtype
Open

Map Calcite ROW to STRUCT in convertSqlTypeNameToExprType#5737
mch2 wants to merge 1 commit into
opensearch-project:mainfrom
mch2:row-to-struct-exprtype

Conversation

@mch2

@mch2 mch2 commented Sep 1, 2026

Copy link
Copy Markdown
Member

Description

convertExprTypeToRelDataType builds STRUCT as MAP<VARCHAR, ANY>, since the v2 path only passes _source JSON through and needs no field types. So the reverse mapping only had case MAP -> STRUCT — a genuine Calcite ROW matched nothing, fell through to default -> UNKNOWN, and surfaced as "type": "unknown" in the response schema.

That is reachable from the analytics engine, which materializes an object field as a real ROW built from typed columns. Before, fields city reported "unknown"; now "struct", matching what a lucene-only cluster returns for the same mapping.

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.

convertExprTypeToRelDataType builds STRUCT as MAP<VARCHAR, ANY>, since the v2
path only passes _source JSON through and needs no field types. So the reverse
mapping only had `case MAP -> STRUCT` — a genuine Calcite ROW matched nothing,
fell through to `default -> UNKNOWN`, and surfaced as "type": "unknown" in the
response schema.

That is reachable from the analytics engine, which materializes an `object`
field as a real ROW built from typed columns. Before, `fields city` reported
"unknown"; now "struct", matching what a lucene-only cluster returns for the
same mapping.

Signed-off-by: Marc Handalian <[email protected]>
Signed-off-by: Marc Handalian <[email protected]>
@github-actions

github-actions Bot commented Sep 1, 2026

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

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Document conversion asymmetry issue

The comment mentions that convertExprTypeToRelDataType builds STRUCT as MAP<VARCHAR,
ANY>, but this creates an inconsistency. When ROW is converted to STRUCT here, and
then STRUCT is converted back via convertExprTypeToRelDataType, it becomes a MAP
instead of a ROW, breaking round-trip conversion. Consider documenting this
asymmetry or ensuring bidirectional consistency.

core/src/main/java/org/opensearch/sql/calcite/utils/OpenSearchTypeFactory.java [255-259]

 // Calcite spells a struct as ROW. convertExprTypeToRelDataType builds STRUCT as
 // MAP<VARCHAR, ANY> since the v2 path only passes _source JSON through, so a genuine ROW —
 // from an engine that builds a struct out of typed columns — matched nothing and fell
 // through to UNKNOWN.
+// Note: This creates an asymmetry where ROW->STRUCT->MAP in round-trip conversions.
 case ROW -> STRUCT;
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a potential round-trip conversion asymmetry between ROW->STRUCT->MAP. However, it only proposes adding a comment rather than fixing the underlying issue, and the existing comment already explains the context. The added note provides marginal value.

Low

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant