Materialize object fields as structs in the analytics engine

[mch2]

Description

You couldn't query an object field directly, for example, (fields city, stats ... by city). The schema flattened objects into dotted leaf columns and never added the parent, so city wasn't a column at all.

Add the object back as a ROW column, then rewrite each scan to read only the leaves and rebuild the object above it in a project, using a new make_struct call. The struct column can't stay in the scan: objects have no physical storage, so FieldStorageResolver rejects it with "Field [city] not found in field storage". Sub-objects get their own nested make_struct.

To get make_struct over to DataFusion we build the Substrait call ourselves instead of letting isthmus match it against a declared signature. Isthmus wants every argument of a variadic function to be the same type, but ours alternate between a string field name and a value of any type, so no variadic declaration matches. Declaring one signature per
field count does match, but that hardcodes the widest struct we support in a YAML file, and an OTel span's attributes already has 55 sub-fields.

Related Issues

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

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

[github-actions]

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

⚡ Recommended focus areas for review Potential name collision The materializer assumes flat leaf columns are named with dotted paths (path + "." + child.getName()), matching the schema builder's convention. If a mapping ever legitimately contains a top-level field whose name is the dotted form of another object's leaf (e.g., a field literally named city.name alongside object city with sub-field name), leafIndexByName would map ambiguously and the struct could bind to the wrong column. Worth confirming the schema builder rejects/escapes such collisions, otherwise the wrong leaf could be silently placed into the struct. private static RexNode buildStruct( RexBuilder rexBuilder, RelNode leafScan, String path, RelDataType structType, Map<String, Integer> leafIndexByName ) { List<String> fieldNames = new ArrayList<>(); List<RexNode> fieldValues = new ArrayList<>(); for (RelDataTypeField child : structType.getFieldList()) { String childPath = path + "." + child.getName(); RexNode value; if (child.getType().isStruct()) { // A child that is itself an object nests another make_struct over its own leaves. value = buildStruct(rexBuilder, leafScan, childPath, child.getType(), leafIndexByName); } else { Integer leafIndex = leafIndexByName.get(childPath); value = leafIndex == null ? null : rexBuilder.makeInputRef(leafScan, leafIndex); } if (value == null) { return null; } fieldNames.add(child.getName()); fieldValues.add(value); } if (fieldNames.isEmpty()) { return null; } return MakeStructFunction.makeCall(rexBuilder, structType, fieldNames, fieldValues); } Nested struct name flattening for RelRoot fields flattenNamesForSubstrait(rootFields, rowType) uses the aliased name for a top-level struct field but then appends the struct's internal child names from rowType — those child names are the original ones, not any aliases. This is fine for typical cases, but if a RelRoot ever aliases a nested struct field name at the top level while children carry different names in the type, the emitted name list will not reflect any aliasing at nested levels. Not a bug given current usage, but a subtle coupling worth documenting or tightening if RelRoot ever provides nested aliases. private static List<String> flattenNamesForSubstrait( List<? extends Map.Entry<Integer, String>> rootFields, RelDataType rowType ) { List<RelDataTypeField> fields = rowType.getFieldList(); List<String> flattened = new ArrayList<>(rootFields.size()); for (Map.Entry<Integer, String> rootField : rootFields) { flattened.add(rootField.getValue()); int index = rootField.getKey(); if (index >= 0 && index < fields.size() && fields.get(index).getType().isStruct()) { appendNestedNames(flattened, fields.get(index).getType()); } } return flattened; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 7ac1a9d
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard group-key index bounds check The group key index refers to a column position on the aggregate's input, but exprs.get(key) is only valid when the key is within bounds. If the group key equals or exceeds exprs.size() (e.g. a projected agg with virtual columns), this throws IndexOutOfBoundsException. Add the same guard used in matches(). sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchAggregateStructKeyRule.java [109-114] Map<Integer, RexCall> expanded = new LinkedHashMap<>(); for (int key : agg.getGroupSet()) { - if (isMakeStruct(exprs.get(key))) { + if (key < exprs.size() && isMakeStruct(exprs.get(key))) { expanded.put(key, (RexCall) exprs.get(key)); } } Suggestion importance[1-10]: 2 __ Why: The matches() method already includes the key < exprs.size() guard, and since onMatch is called after matches returns true, in practice the group keys refer to project outputs. The suggestion adds defensive redundancy but is not fixing a real bug given the operand structure Aggregate(Project).	Low
General	Validate root-field index bounds explicitly When a root field's type is a struct, only the top-level struct field names are appended, but appendNestedNames correctly recurses. However, if a struct has zero fields, no names are appended — verify this cannot occur (buildObjectType returns null for empty structs, so it should be fine, but a defensive check on the total flattened count vs. the Substrait NamedStruct expected count would help catch schema drift). sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [657-668] private static List<String> flattenNamesForSubstrait(List<? extends Map.Entry<Integer, String>> rootFields, RelDataType rowType) { List<RelDataTypeField> fields = rowType.getFieldList(); List<String> flattened = new ArrayList<>(rootFields.size()); for (Map.Entry<Integer, String> rootField : rootFields) { flattened.add(rootField.getValue()); int index = rootField.getKey(); - if (index >= 0 && index < fields.size() && fields.get(index).getType().isStruct()) { + if (index < 0 || index >= fields.size()) { + throw new IllegalStateException("Root field index " + index + " out of range for row type of size " + fields.size()); + } + if (fields.get(index).getType().isStruct()) { appendNestedNames(flattened, fields.get(index).getType()); } } return flattened; } Suggestion importance[1-10]: 2 __ Why: The existing bounds check is defensive already; the suggestion converts it into an exception, which is a minor style/defensiveness change and asks the author to "verify" schema drift. Low impact.	Low
	Verify nested object parents are exposed fieldName here is the local key (e.g. "nested_metadata") at the top-level call, but inside recursive addLeafFields calls it is passed as the parent prefix. Verify the exposed struct column name is the fully qualified dotted path so nested objects (an object inside an object) get addressable parent columns too — otherwise only the top-level parent is exposed, and mid-level parents are silently omitted from the row type. sandbox/libs/analytics-api/src/main/java/org/opensearch/analytics/schema/OpenSearchSchemaBuilder.java [352-367] if (fieldType == null || "object".equals(fieldType)) { Map<String, Object> nested = (Map<String, Object>) fieldProps.get("properties"); if (nested != null) { addLeafFields(builder, typeFactory, nested, fieldName); - // Also expose the object itself as a struct (ROW) column, so a query can - // address the whole object (`fields nested_metadata`, `stats … by obj`) and - // not just its leaves. The object has no physical storage — the scan reads - // the leaves — so ObjectStructMaterializer strips this column from the scan - // and re-assembles it with make_struct in a project directly above it. RelDataType structType = buildObjectType(typeFactory, nested, fieldName); if (structType != null) { + // fieldName already carries the dotted path from the caller's prefix. builder.add(fieldName, structType); } } continue; } Suggestion importance[1-10]: 1 __ Why: The suggestion only asks the author to verify existing behavior without proposing a concrete code change (the improved_code is essentially identical, just with a comment). The fieldName prefix is indeed the dotted path per the recursion. Low value.	Low

---

Previous suggestions

Suggestions up to commit a3732e8

Category	Suggestion	Impact
Possible issue	Use RelRoot's own row type for lookup root.fields uses RelRoot's output field mapping whose keys are indices into root.rel.getRowType(), not preprocessed.getRowType(). Since preprocessed IS the input to RelRoot.of(preprocessed, ...), they should coincide, but if RelRoot.of ever wraps or re-derives, the indices may not align. Pass root.rel.getRowType() (or root.validatedRowType) instead of preprocessed.getRowType() to guarantee the index-to-type lookup remains correct. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [590] -List<String> fieldNames = flattenNamesForSubstrait(root.fields, preprocessed.getRowType()); +List<String> fieldNames = flattenNamesForSubstrait(root.fields, root.rel.getRowType()); Suggestion importance[1-10]: 5 __ Why: A reasonable defensive suggestion — using root.rel.getRowType() is more semantically correct as the indices in root.fields refer to that row type. However, since root = RelRoot.of(preprocessed, ...), they coincide in practice, so the impact is minor.	Low
General	Ensure all scan subtypes are visited RelShuttleImpl.visit(TableScan) is only invoked for direct scan children via generic visiting. If a scan is wrapped in a subclass not covered by the shuttle's dispatch (e.g., a custom TableScan subtype), it may bypass this rewrite. Consider also overriding visit(RelNode other) or ensuring the shuttle traverses through any custom scan nodes so this materialization is not silently skipped in plans containing non-LogicalTableScan scans. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/ObjectStructMaterializer.java [101-106] +@Override +public RelNode visit(TableScan scan) { + RelDataType originalRowType = scan.getRowType(); + List<RelDataTypeField> originalFields = originalRowType.getFieldList(); + if (originalFields.stream().noneMatch(f -> f.getType().isStruct())) { + return scan; + } - Suggestion importance[1-10]: 3 __ Why: The suggestion raises a theoretical concern about custom TableScan subtypes not being dispatched, but RelShuttleImpl does dispatch to visit(TableScan) for all TableScan subclasses via its visit(RelNode) method. The improved_code is identical to the existing code.	Low
	Make dataset provisioning idempotent The dataProvisioned static flag persists across test classes in the same JVM but is only set for this dataset, which is fine — however, if this test class is re-instantiated in a fresh JVM after a previous run's index still exists, provisioning may fail. Also, since JUnit may run tests in parallel or reload classes, guard with proper idempotency or check for index existence rather than a static boolean. sandbox/plugins/analytics-engine/src/test/java/org/opensearch/analytics/qa/ApmServiceMapObjectIT.java [47-55] +private static boolean dataProvisioned = false; +@Override +protected void onBeforeQuery() throws IOException { + if (dataProvisioned == false) { + DatasetProvisioner.provision(client(), DATASET); + dataProvisioned = true; + } +} Suggestion importance[1-10]: 2 __ Why: The suggestion is vague, the improved_code is identical to existing_code, and it only speculates about potential issues without concrete fixes.	Low
	Verify nested object exposure at all levels When the recursive call reaches nested objects, fieldName is passed as the prefix but addLeafFields prepends it to build the dotted path. For nested objects at deeper levels, ensure buildObjectType is called with the correct full dotted pathPrefix matching what the leaf columns use — currently the top-level call passes fieldName which is only the local name at the top scope, but for recursive addLeafFields invocations with a prefix, the nested object exposure uses the compound path, which is fine. However, verify that when addLeafFields recurses (line above), the nested sub-object is not also exposed here at the top — it should be exposed at every level, which this code does correctly only at leaf-recursion depth. sandbox/libs/analytics-api/src/main/java/org/opensearch/analytics/schema/OpenSearchSchemaBuilder.java [352-366] if (fieldType == null || "object".equals(fieldType)) { Map<String, Object> nested = (Map<String, Object>) fieldProps.get("properties"); if (nested != null) { addLeafFields(builder, typeFactory, nested, fieldName); - // Also expose the object itself as a struct (ROW) column, so a query can - // address the whole object (`fields nested_metadata`, `stats … by obj`) and - // not just its leaves. The object has no physical storage — the scan reads - // the leaves — so ObjectStructMaterializer strips this column from the scan - // and re-assembles it with make_struct in a project directly above it. RelDataType structType = buildObjectType(typeFactory, nested, fieldName); if (structType != null) { builder.add(fieldName, structType); } } continue; } Suggestion importance[1-10]: 2 __ Why: The suggestion asks to verify existing behavior and offers essentially identical code without a concrete change. It reads as a verification/observation rather than a fix.	Low

[github-actions]

✅ Gradle check result for a3732e8: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.61%. Comparing base (2914470) to head (7ac1a9d).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22864      +/-   ##
============================================
+ Coverage     71.57%   71.61%   +0.04%     
- Complexity    77269    77341      +72     
============================================
  Files          6170     6170              
  Lines        359774   359870      +96     
  Branches      52478    52487       +9     
============================================
+ Hits         257504   257717     +213     
+ Misses        81801    81644     -157     
- Partials      20469    20509      +40     

☔ 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.

[github-actions]

✅ Gradle check result for 7ac1a9d: SUCCESS