[Feature] flat_object Type Support

[RyanL1997]

Description

A flat_object field is invisible to PPL today: parseMapping skips the type, so describe does not list it and any reference to it fails with field not found.

Scope

The field is presented as a struct keyed by the dotted path of each leaf, read from _source and flattened, so either spelling of a path ({"a": {"b": 1}} or {"a.b": 1}) resolves to the same entry. Each leaf keeps the type it was written with — the map's value type is Calcite's VARIANT — so 12.5 comes back as a number, "4" as text, and an array leaf as an array whose elements keep their own types.

What a query may do with a leaf is bounded by the field's index, not by PPL: a flat_object indexes every leaf value as a keyword term with the path folded in and gives the leaf no mapping of its own (field type docs). Hence the rule — every filter the index answers is supported and pushed down as the same request a DSL user writes; nothing is answered by a script or by scanning _source. RexStandardizer and ScriptQueryExpression refuse to build a script over a flat_object at all, so a plan that would need one falls back rather than opening every record.

Supported, because the DSL answers it from the index

PPL	pushed down as
fields attributes, fields attributes.namespace, eval ns = attributes.namespace	_source include
where attributes.namespace = 'prod', where attributes.status_code = 503	term
where attributes.namespace != 'prod'	exists + must_not term
where attributes.namespace in ('prod', 'staging')	one term per value under should
where isnotnull(attributes.k) / where isnull(attributes.k)	exists / must_not exists
where like(attributes.namespace, 'ns-%'), where like(attributes.namespace, '%prod%')	wildcard
where not like(attributes.namespace, 'ns-%')	exists + must_not wildcard
the above with and / or / not, and with filters on other fields	bool

Two consequences, both the DSL's own behaviour: a lookup is a lookup of the term, so it does not distinguish a value from the text that spells it (= '503' and = 503 both match a record that wrote either; = 'true' matches one that wrote the boolean); and like pushes down as the query a mapped keyword field gets, byte for byte — {"wildcard":{"<path>":{"wildcard":"ns-*","case_insensitive":true}}}.

Not supported, because the DSL cannot answer it from the index

PPL	why
stats avg(attributes.duration_ms), and any other aggregate	OpenSearch refuses: Field [...] of type [flat_object] is not supported for aggregation
stats count() by attributes.k	a terms aggregation over the folded terms returns bucket keys like java.lang.Object@21c06f0
sort attributes.k	no doc values: Field [...] of type [flat_object] does not support doc_value in root field
computing a new value from a leaf -- a function, an expression, or a cast that returns the value	computed from the value rather than looked up; a cast inside a supported filter is unaffected
where attributes.duration_ms > 10, and < >= <= between	the index would answer it, but wrongly: the terms are text, so a range query does not match the leaf holding 200 for > 90

The last row is the only place PPL is deliberately stricter than the DSL, and its error says why.

Error messages

Raised through ErrorReport — the way AnalyticsEngineFormatSupport and the alias check in OpenSearchDataType raise an unsupported operation — rather than as a message assembled by hand. The cause says only what the query did; the rule and the doc link ride on the report's suggestion:

{
  "error": {
    "reason": "Cannot sort by attributes.duration_ms",
    "code": "UNSUPPORTED_OPERATION",
    "suggestion": "A flat_object field can only be projected, or looked up in the index -- by value, by existence or by a pattern. See https://opensearch.org/docs/latest/field-types/supported-field-types/flat-object/",
    "context": { "stage": "analyzing", "stage_description": "Parsing and validating the query" },
    "details": "Cannot sort by attributes.duration_ms",
    "location": [
      "while preparing and validating the query plan",
      "while checking what a flat_object field can answer"
    ],
    "type": "IllegalArgumentException"
  },
  "status": 400
}

Same shape, same suggestion, for Cannot compare attributes.duration_ms with >, Cannot group by attributes.k, Cannot apply avg to a flat_object leaf, Cannot evaluate an expression over attributes.k, and Cannot apply typeof to a flat_object leaf. Nothing in a message is invented — only the field name and the operator, both read off the query.

expand over a leaf gets its own suggestion, since the fix is different: expand takes a field of the index, not a value computed from one. Expand the field itself, or project the value first.

How

FlatObjectScopeValidator is a pass over the logical plan, like withCheckedArithmetic, run from QueryService.analyze. It recognizes the field and its leaves by the types they are registered with — MAP<VARCHAR, VARIANT> and VARIANT — read off each node's input row type, and walks the plan with Calcite's RelVisitor. Functions that receive a leaf they cannot take raise the same message from the function resolver rather than a type error. A leaf also renders as the text the index holds for it, so a filter evaluated on the coordinator (plugins.calcite.pushdown.enabled=false) returns the same rows as the same filter pushed down.

One behaviour that follows the DSL and is documented as such: a path through an array of objects can be filtered (where attributes.items.name = 'alpha' — the index holds one term per element) but projects as null, since in the document the value lives inside the array that fields attributes.items returns whole.

Related Issues

Related to #1604, #3067

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit d2aa48f)

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 Possible Issue The recursion depth guard uses depth < MAX_DEPTH to descend, but MAX_DEPTH is 20, meaning the code descends to depth 19 (0-indexed) and stops at 20. The comment states "Levels beyond the cap are kept as one leaf," but the test expects 21 levels of keys (0-20) before stopping. If the intent is to allow 20 levels of nesting, the condition should be depth <= MAX_DEPTH or MAX_DEPTH should be 19. This discrepancy could cause objects at exactly depth 20 to be serialized as strings when they should be flattened, or vice versa. if (value.isObject() && depth < MAX_DEPTH) { flattenInto(value, key, out, depth + 1); return; Possible Issue The cast method for Array always returns null regardless of the target type. This means any attempt to cast an array variant to another type silently fails. If array variants are expected to be castable (e.g., to a string representation or to extract elements), this implementation will cause unexpected null results. If arrays are intentionally not castable, the method should document this or throw an exception to make the limitation explicit. public @Nullable Object cast(RuntimeTypeInformation type) { return null; } Possible Issue In flatObjectLeafPath, the method checks if column is null after retrieving it from the schema, but does not verify that ref.getIndex() is within bounds before accessing schema.get(ref.getIndex()). If ref.getIndex() >= schema.size(), the ternary will assign null to column, but the real issue is an out-of-bounds index that should not occur. This could mask a logic error where an invalid index is passed. Consider adding an explicit bounds check or assertion. String column = ref.getIndex() < schema.size() ? schema.get(ref.getIndex()) : null; if (column == null || !(fieldTypes.get(column) instanceof OpenSearchFlatObjectType)) {

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to d2aa48f

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle null before string conversion Using String.valueOf() on arbitrary objects may produce unexpected results for complex types. Consider handling null values explicitly before conversion to ensure consistent behavior. core/src/main/java/org/opensearch/sql/data/model/ExprVariantValue.java [47-56] @Override public @Nullable Object cast(RuntimeTypeInformation type) { if (type.getTypeName() == RuntimeSqlTypeName.VARCHAR) { - // The index holds every leaf as the text of the value it was written with -- the term for - // 12.5 is "12.5" and for true is "true" -- so a leaf read here must render as that same - // text. Calcite's own cast answers null for a leaf not written as text, which would make a - // filter evaluated here disagree with the one pushed down as a term query. + if (value == null) { + return null; + } return String.valueOf(value); } return delegate.cast(type); } Suggestion importance[1-10]: 6 __ Why: Adding explicit null handling before String.valueOf() improves code clarity and prevents potential edge cases, though String.valueOf() already handles null by returning "null". The improvement is minor but valid for defensive programming.	Low
Possible issue	Add index bounds validation Add bounds checking before accessing inputFields by index to prevent potential IndexOutOfBoundsException. The code assumes indices are always valid but doesn't verify this assumption. core/src/main/java/org/opensearch/sql/calcite/FlatObjectScopeValidator.java [92-113] private static void check(RelNode node) { List<RelDataTypeField> inputFields = inputFields(node); - // Sort keys, group keys, aggregate arguments and the column an expand correlates on are - // positions, not expressions, so they are checked against the input row type directly. if (node instanceof Sort sort) { for (var key : sort.getCollation().getFieldCollations()) { - rejectIfFlatObject(node, inputFields, key.getFieldIndex(), "Cannot sort by %s"); + int index = key.getFieldIndex(); + if (index >= 0 && index < inputFields.size()) { + rejectIfFlatObject(node, inputFields, index, "Cannot sort by %s"); + } } return; } if (node instanceof Aggregate aggregate) { for (int key : aggregate.getGroupSet()) { - rejectIfFlatObject(node, inputFields, key, "Cannot group by %s"); + if (key >= 0 && key < inputFields.size()) { + rejectIfFlatObject(node, inputFields, key, "Cannot group by %s"); + } } for (AggregateCall call : aggregate.getAggCallList()) { for (int arg : call.getArgList()) { - rejectIfFlatObject( - node, - inputFields, - arg, - "Cannot apply " - + call.getAggregation().getName().toLowerCase(Locale.ROOT) - + " to %s"); + if (arg >= 0 && arg < inputFields.size()) { + rejectIfFlatObject( + node, + inputFields, + arg, + "Cannot apply " + + call.getAggregation().getName().toLowerCase(Locale.ROOT) + + " to %s"); + } } } return; } ... Suggestion importance[1-10]: 3 __ Why: While bounds checking is generally good practice, Calcite's internal structures should guarantee valid indices. The added checks would be defensive but may be unnecessary overhead. The suggestion is valid but addresses a low-probability edge case.	Low

---

Previous suggestions

Suggestions up to commit 9ce89fd

Category	Suggestion	Impact
Possible issue	Add recursion depth guard for arrays The recursive call to toVariant for array elements has no depth guard, which could cause stack overflow for deeply nested arrays. Add a depth parameter to prevent unbounded recursion. core/src/main/java/org/opensearch/sql/data/model/ExprVariantTupleValue.java [54-67] static @Nullable VariantValue toVariant(ExprValue leaf) { + return toVariant(leaf, 0); +} + +private static @Nullable VariantValue toVariant(ExprValue leaf, int depth) { if (leaf.isNull()) { return null; + } + if (depth > MAX_RECURSION_DEPTH) { + throw new IllegalStateException("Maximum recursion depth exceeded for variant conversion"); } if (leaf instanceof ExprCollectionValue array) { List<@Nullable VariantValue> elements = new ArrayList<>(); for (ExprValue element : array.collectionValue()) { - elements.add(toVariant(element)); + elements.add(toVariant(element, depth + 1)); } return new ExprVariantValue.Array(elements); } return new ExprVariantValue( ROUNDING_MODE, leaf.valueForCalcite(), new BasicSqlTypeRtti(runtimeTypeOf(leaf.type()))); } Suggestion importance[1-10]: 7 __ Why: This is a valid concern about unbounded recursion in toVariant for deeply nested arrays. While FlatObjectValues has a MAX_DEPTH guard, toVariant processes already-parsed values and could encounter deeply nested arrays. Adding a depth guard would prevent potential stack overflow issues, though such deeply nested structures may be rare in practice.	Medium
General	Document the depth limit rationale The depth limit of 20 is hardcoded without clear justification. Consider making this configurable or documenting why this specific value was chosen, especially since flat_object fields exist to escape mapping depth limits. opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/FlatObjectValues.java [29-35] +/** + * Maximum depth for flattening nested objects. Beyond this depth, nested structures are kept + * as a single string leaf to prevent excessive recursion and memory usage. + */ static final int MAX_DEPTH = 20; Suggestion importance[1-10]: 5 __ Why: Adding documentation for MAX_DEPTH would improve code clarity. The existing comment mentions it's a "depth guard" but doesn't explain why 20 was chosen. However, the suggestion only adds a comment without changing functionality, so the impact is moderate.	Low
	Remove early returns for comprehensive validation The method returns early after checking each node type, which means nodes with multiple inputs (like joins) may not have all their expressions validated. Consider removing early returns and allowing the final RexShuttle to run for all node types to ensure comprehensive validation. core/src/main/java/org/opensearch/sql/calcite/FlatObjectScopeValidator.java [88-152] private static void check(RelNode node) { List<RelDataTypeField> inputFields = inputFields(node); // Sort keys, group keys, aggregate arguments and the column an expand correlates on are // positions, not expressions, so they are checked against the input row type directly. if (node instanceof Sort sort) { for (var key : sort.getCollation().getFieldCollations()) { rejectIfFlatObject(node, inputFields, key.getFieldIndex(), "Cannot sort by %s"); } - return; - } - if (node instanceof Aggregate aggregate) { + } else if (node instanceof Aggregate aggregate) { for (int key : aggregate.getGroupSet()) { rejectIfFlatObject(node, inputFields, key, "Cannot group by %s"); } for (AggregateCall call : aggregate.getAggCallList()) { for (int arg : call.getArgList()) { rejectIfFlatObject( node, inputFields, arg, "Cannot apply " + call.getAggregation().getName().toLowerCase(Locale.ROOT) + " to %s"); } } - return; - } - if (node instanceof Correlate correlate) { + } else if (node instanceof Correlate correlate) { for (int required : correlate.getRequiredColumns()) { rejectIfFlatObject(node, inputFields, required, "Cannot expand %s"); } - return; + } else if (node instanceof Project project) { + ... + } else if (node instanceof Filter filter && filter.getCondition() instanceof RexCall condition) { + ... + } else { + // Any other node -- a join condition, a window, a command with expressions of its own + node.accept( + new RexShuttle() { + @Override + public RexNode visitInputRef(RexInputRef ref) { + if (isFlatObjectOrLeaf(inputFields.get(ref.getIndex()).getType())) { + throw unsupported("Cannot use %s here", name(inputFields, ref.getIndex())); + } + return ref; + } + }); } - ... } Suggestion importance[1-10]: 3 __ Why: The early returns are intentional: Sort, Aggregate, Correlate, Project, and Filter nodes have specialized validation logic that handles their specific structures. The final RexShuttle is only for nodes without specialized handling. The current design is correct and changing it would not improve validation coverage.	Low

Suggestions up to commit 00fa3f0

Category	Suggestion	Impact
General	Document depth limit rationale The hardcoded depth limit of 20 may be insufficient for deeply nested documents and could silently truncate data. Consider making this configurable or documenting the rationale for this specific value. opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/FlatObjectValues.java [34] +// Maximum nesting depth for flat_object fields. Beyond this depth, nested structures +// are kept as a single leaf to prevent stack overflow while processing malicious documents. +// This limit balances between supporting legitimate use cases and preventing resource exhaustion. static final int MAX_DEPTH = 20; Suggestion importance[1-10]: 5 __ Why: Adding documentation for MAX_DEPTH improves code maintainability by explaining why this specific limit exists. However, the existing comment already mentions it's a "depth guard" to prevent unbounded recursion, so this is a minor enhancement.	Low
	Handle null in cast operation Using String.valueOf() on arbitrary objects may produce unexpected results for complex types. Consider adding explicit handling for null values and arrays to ensure consistent string representation. core/src/main/java/org/opensearch/sql/data/model/ExprVariantValue.java [47-56] @Override public @Nullable Object cast(RuntimeTypeInformation type) { if (type.getTypeName() == RuntimeSqlTypeName.VARCHAR) { + if (value == null) { + return null; + } // The index holds every leaf as the text of the value it was written with -- the term for // 12.5 is "12.5" and for true is "true" -- so a leaf read here must render as that same // text. Calcite's own cast answers null for a leaf not written as text, which would make a // filter evaluated here disagree with the one pushed down as a term query. return String.valueOf(value); } return delegate.cast(type); } Suggestion importance[1-10]: 2 __ Why: The suggestion to add explicit null handling is reasonable, but String.valueOf(null) already returns "null" in Java, which may be the intended behavior for matching index terms. The suggestion doesn't clearly improve correctness without understanding the specific requirements for null representation.	Low
Possible issue	Add index bounds validation Add bounds checking before accessing inputFields by index to prevent potential IndexOutOfBoundsException. The code assumes indices are always valid, but malformed plans or edge cases could provide out-of-range indices. core/src/main/java/org/opensearch/sql/calcite/FlatObjectScopeValidator.java [92-114] private static void check(RelNode node) { List<RelDataTypeField> inputFields = inputFields(node); - // Sort keys, group keys, aggregate arguments and the column an expand correlates on are - // positions, not expressions, so they are checked against the input row type directly. if (node instanceof Sort sort) { for (var key : sort.getCollation().getFieldCollations()) { - rejectIfFlatObject(node, inputFields, key.getFieldIndex(), "Cannot sort by %s"); + int index = key.getFieldIndex(); + if (index >= 0 && index < inputFields.size()) { + rejectIfFlatObject(node, inputFields, index, "Cannot sort by %s"); + } } return; } if (node instanceof Aggregate aggregate) { for (int key : aggregate.getGroupSet()) { - rejectIfFlatObject(node, inputFields, key, "Cannot group by %s"); + if (key >= 0 && key < inputFields.size()) { + rejectIfFlatObject(node, inputFields, key, "Cannot group by %s"); + } } for (AggregateCall call : aggregate.getAggCallList()) { for (int arg : call.getArgList()) { - rejectIfFlatObject( - node, - inputFields, - arg, - "Cannot apply " - + call.getAggregation().getName().toLowerCase(Locale.ROOT) - + " to %s"); + if (arg >= 0 && arg < inputFields.size()) { + rejectIfFlatObject( + node, + inputFields, + arg, + "Cannot apply " + + call.getAggregation().getName().toLowerCase(Locale.ROOT) + + " to %s"); + } } } return; } ... Suggestion importance[1-10]: 3 __ Why: While bounds checking is generally good practice, Calcite's internal structures are expected to maintain consistency between field indices and the row type. Adding these checks would guard against malformed plans but may be unnecessary overhead for a well-formed system.	Low

Suggestions up to commit 34340bb

Category	Suggestion	Impact
General	Handle null value in cast Handle null value explicitly before calling String.valueOf() to avoid returning the string "null" instead of an actual null value, which could cause incorrect query results. core/src/main/java/org/opensearch/sql/data/model/ExprVariantValue.java [47-56] @Override public @Nullable Object cast(RuntimeTypeInformation type) { if (type.getTypeName() == RuntimeSqlTypeName.VARCHAR) { - // The index holds every leaf as the text of the value it was written with -- the term for - // 12.5 is "12.5" and for true is "true" -- so a leaf read here must render as that same - // text. Calcite's own cast answers null for a leaf not written as text, which would make a - // filter evaluated here disagree with the one pushed down as a term query. + if (value == null) { + return null; + } return String.valueOf(value); } return delegate.cast(type); } Suggestion importance[1-10]: 8 __ Why: This is a valid concern. If value is null, String.valueOf(null) returns the string "null" rather than null itself, which would cause incorrect behavior when casting a null variant to VARCHAR. The suggestion correctly identifies this potential bug and provides the appropriate fix.	Medium
	Fix off-by-one in depth limit The recursion depth limit should be enforced consistently. Currently MAX_DEPTH is checked in fold() but the initial call to flattenInto() starts at depth 0, allowing 21 levels instead of 20 as documented. opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/FlatObjectValues.java [34] -static final int MAX_DEPTH = 20; +static final int MAX_DEPTH = 19; Suggestion importance[1-10]: 3 __ Why: The suggestion misunderstands the implementation. The check depth < MAX_DEPTH in fold() at line 88 correctly limits recursion to 20 levels (0-19), and the comment at line 33 states "Levels beyond the cap are kept as one leaf," which is the intended behavior. The current implementation is correct.	Low
Possible issue	Add index bounds validation Add bounds checking before accessing inputFields by index to prevent potential IndexOutOfBoundsException. The code assumes indices are always valid but should verify index < inputFields.size() before access. core/src/main/java/org/opensearch/sql/calcite/FlatObjectScopeValidator.java [92-114] private static void check(RelNode node) { List<RelDataTypeField> inputFields = inputFields(node); - // Sort keys, group keys, aggregate arguments and the column an expand correlates on are - // positions, not expressions, so they are checked against the input row type directly. if (node instanceof Sort sort) { for (var key : sort.getCollation().getFieldCollations()) { - rejectIfFlatObject(node, inputFields, key.getFieldIndex(), "Cannot sort by %s"); + int index = key.getFieldIndex(); + if (index >= 0 && index < inputFields.size()) { + rejectIfFlatObject(node, inputFields, index, "Cannot sort by %s"); + } } return; } if (node instanceof Aggregate aggregate) { for (int key : aggregate.getGroupSet()) { - rejectIfFlatObject(node, inputFields, key, "Cannot group by %s"); + if (key >= 0 && key < inputFields.size()) { + rejectIfFlatObject(node, inputFields, key, "Cannot group by %s"); + } } for (AggregateCall call : aggregate.getAggCallList()) { for (int arg : call.getArgList()) { - rejectIfFlatObject( - node, - inputFields, - arg, - "Cannot apply " - + call.getAggregation().getName().toLowerCase(Locale.ROOT) - + " to %s"); + if (arg >= 0 && arg < inputFields.size()) { + rejectIfFlatObject( + node, + inputFields, + arg, + "Cannot apply " + + call.getAggregation().getName().toLowerCase(Locale.ROOT) + + " to %s"); + } } } return; } ... Suggestion importance[1-10]: 7 __ Why: Adding bounds checking is a defensive programming practice that prevents potential IndexOutOfBoundsException. However, in Calcite's relational algebra, field indices in Sort, Aggregate, and AggregateCall are typically guaranteed to be valid by the planner, making this a low-probability issue.	Medium

Suggestions up to commit ce07556

Category	Suggestion	Impact
Possible issue	Add recursion depth guard The recursive call to toVariant within the array handling has no depth guard, which could cause stack overflow for deeply nested arrays. Add a depth parameter and limit to prevent unbounded recursion. core/src/main/java/org/opensearch/sql/data/model/ExprVariantTupleValue.java [54-67] static @Nullable VariantValue toVariant(ExprValue leaf) { + return toVariant(leaf, 0); +} + +private static @Nullable VariantValue toVariant(ExprValue leaf, int depth) { if (leaf.isNull()) { return null; + } + if (depth > FlatObjectValues.MAX_DEPTH) { + throw new IllegalStateException("Maximum nesting depth exceeded for variant conversion"); } if (leaf instanceof ExprCollectionValue array) { List<@Nullable VariantValue> elements = new ArrayList<>(); for (ExprValue element : array.collectionValue()) { - elements.add(toVariant(element)); + elements.add(toVariant(element, depth + 1)); } return new ExprVariantValue.Array(elements); } return new ExprVariantValue( ROUNDING_MODE, leaf.valueForCalcite(), new BasicSqlTypeRtti(runtimeTypeOf(leaf.type()))); } Suggestion importance[1-10]: 7 __ Why: This is a valid concern. The toVariant method recursively processes arrays without depth protection, which could cause stack overflow for deeply nested arrays. Adding a depth guard similar to FlatObjectValues.MAX_DEPTH would improve robustness.	Medium
General	Document depth limit rationale The MAX_DEPTH constant of 20 is hardcoded without explanation of why this specific value was chosen. Consider making this configurable or documenting the rationale, as different use cases may require different depth limits. opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/FlatObjectValues.java [34] +/** + * Maximum depth for flattening nested objects. Levels beyond this are kept as string leaves + * to prevent stack overflow and excessive memory usage. This value balances between supporting + * deeply nested documents and maintaining system stability. + */ static final int MAX_DEPTH = 20; Suggestion importance[1-10]: 5 __ Why: Adding documentation for MAX_DEPTH would improve code clarity. The existing comment mentions why a limit exists but doesn't explain the specific value of 20. However, the suggestion to make it configurable may be unnecessary complexity.	Low
	Clarify max depth handling When depth >= MAX_DEPTH, objects are treated as scalar values and parsed directly. This could lead to incorrect data representation for deeply nested structures. Consider logging a warning or explicitly handling this edge case to make the depth limit behavior more transparent. opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/FlatObjectValues.java [86-96] private static void fold( Content value, String key, LinkedHashMap<String, ExprValue> out, int depth) { if (value.isObject() && depth < MAX_DEPTH) { flattenInto(value, key, out, depth + 1); + return; + } + if (value.isObject() && depth >= MAX_DEPTH) { + // Object exceeds max depth - store as string representation + put(out, key, OpenSearchExprValueFactory.parseContent(value)); return; } if (value.isArray()) { value.array().forEachRemaining(element -> fold(element, key, out, depth)); return; } put(out, key, OpenSearchExprValueFactory.parseContent(value)); } Suggestion importance[1-10]: 4 __ Why: The suggestion to explicitly handle the depth >= MAX_DEPTH case for objects could improve code clarity, but the current implementation already handles this correctly by falling through to parseContent. The improved code adds redundancy without significant benefit.	Low
	Remove early returns in validation The check method returns early after processing each node type, which means nodes with multiple concerns (e.g., a Sort with a Filter child) only validate the top-level node. Consider removing early returns and allowing all validation logic to execute, or explicitly document why partial validation is sufficient. core/src/main/java/org/opensearch/sql/calcite/FlatObjectScopeValidator.java [88-120] private static void check(RelNode node) { List<RelDataTypeField> inputFields = inputFields(node); // Sort keys, group keys, aggregate arguments and the column an expand correlates on are // positions, not expressions, so they are checked against the input row type directly. if (node instanceof Sort sort) { for (var key : sort.getCollation().getFieldCollations()) { rejectIfFlatObject(node, inputFields, key.getFieldIndex(), "Cannot sort by %s"); } - return; } if (node instanceof Aggregate aggregate) { for (int key : aggregate.getGroupSet()) { rejectIfFlatObject(node, inputFields, key, "Cannot group by %s"); } for (AggregateCall call : aggregate.getAggCallList()) { for (int arg : call.getArgList()) { rejectIfFlatObject( node, inputFields, arg, "Cannot apply " + call.getAggregation().getName().toLowerCase(Locale.ROOT) + " to %s"); } } - return; } - ... + // Continue with other node type checks... Suggestion importance[1-10]: 3 __ Why: The early returns are intentional and correct. Each node type requires different validation logic, and the RelVisitor in validate() ensures all nodes in the tree are visited. The suggestion misunderstands the visitor pattern being used.	Low

Suggestions up to commit 99d3400

Category	Suggestion	Impact
General	Handle null value correctly Handle potential null value explicitly in the VARCHAR cast to avoid returning the string "null" for actual null values. Use a null check to preserve null semantics correctly. core/src/main/java/org/opensearch/sql/data/model/ExprVariantValue.java [47-56] @Override public @Nullable Object cast(RuntimeTypeInformation type) { if (type.getTypeName() == RuntimeSqlTypeName.VARCHAR) { // The index holds every leaf as the text of the value it was written with -- the term for // 12.5 is "12.5" and for true is "true" -- so a leaf read here must render as that same // text. Calcite's own cast answers null for a leaf not written as text, which would make a // filter evaluated here disagree with the one pushed down as a term query. - return String.valueOf(value); + return value == null ? null : String.valueOf(value); } return delegate.cast(type); } Suggestion importance[1-10]: 7 __ Why: Valid concern about null handling. The constructor accepts Object value, and if value is null, String.valueOf(null) returns "null" rather than null, which could cause incorrect behavior when casting to VARCHAR.	Medium
Possible issue	Add index bounds validation Add bounds checking before accessing inputFields by index to prevent potential IndexOutOfBoundsException. The rejectIfFlatObject method checks index < inputFields.size(), but the callers don't verify this before passing indices. core/src/main/java/org/opensearch/sql/calcite/FlatObjectScopeValidator.java [92-119] private static void check(RelNode node) { List<RelDataTypeField> inputFields = inputFields(node); // Sort keys, group keys, aggregate arguments and the column an expand correlates on are // positions, not expressions, so they are checked against the input row type directly. if (node instanceof Sort sort) { for (var key : sort.getCollation().getFieldCollations()) { - rejectIfFlatObject(node, inputFields, key.getFieldIndex(), "Cannot sort by %s"); + int index = key.getFieldIndex(); + if (index >= 0 && index < inputFields.size()) { + rejectIfFlatObject(node, inputFields, index, "Cannot sort by %s"); + } } return; } if (node instanceof Aggregate aggregate) { for (int key : aggregate.getGroupSet()) { - rejectIfFlatObject(node, inputFields, key, "Cannot group by %s"); + if (key >= 0 && key < inputFields.size()) { + rejectIfFlatObject(node, inputFields, key, "Cannot group by %s"); + } } for (AggregateCall call : aggregate.getAggCallList()) { for (int arg : call.getArgList()) { - rejectIfFlatObject( - node, - inputFields, - arg, - "Cannot apply " - + call.getAggregation().getName().toLowerCase(Locale.ROOT) - + " to %s"); + if (arg >= 0 && arg < inputFields.size()) { + rejectIfFlatObject( + node, + inputFields, + arg, + "Cannot apply " + + call.getAggregation().getName().toLowerCase(Locale.ROOT) + + " to %s"); + } } } return; } if (node instanceof Correlate correlate) { for (int required : correlate.getRequiredColumns()) { - rejectIfFlatObject(node, inputFields, required, "Cannot expand %s"); + if (required >= 0 && required < inputFields.size()) { + rejectIfFlatObject(node, inputFields, required, "Cannot expand %s"); + } } return; } ... } Suggestion importance[1-10]: 3 __ Why: The rejectIfFlatObject method already includes bounds checking (index < inputFields.size()), making the suggested validation redundant. The indices come from Calcite's internal structures which should be valid.	Low
	Add null safety check Add null check for value parameter before calling methods on it to prevent NullPointerException. The method assumes value is non-null but doesn't validate this assumption. opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/FlatObjectValues.java [86-97] private static void fold( Content value, String key, LinkedHashMap<String, ExprValue> out, int depth) { + if (value == null) { + return; + } if (value.isObject() && depth < MAX_DEPTH) { flattenInto(value, key, out, depth + 1); return; } if (value.isArray()) { value.array().forEachRemaining(element -> fold(element, key, out, depth)); return; } put(out, key, OpenSearchExprValueFactory.parseContent(value)); } Suggestion importance[1-10]: 2 __ Why: The fold method is private and called only from flattenInto, which iterates over map entries where values should not be null. Adding a null check is defensive but unlikely to prevent actual issues.	Low

[github-actions]

Persistent review updated to latest commit a58c3db

[codecov]

Codecov Report

❌ Patch coverage is 68.46361% with 117 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.54%. Comparing base (c634b85) to head (d2aa48f).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...ensearch/sql/calcite/FlatObjectScopeValidator.java	68.42%	16 Missing and 26 partials ⚠️
...arch/sql/opensearch/request/PredicateAnalyzer.java	72.85%	6 Missing and 13 partials ⚠️
.../opensearch/sql/calcite/CalciteRelNodeVisitor.java	0.00%	15 Missing ⚠️
...rg/opensearch/sql/data/model/ExprVariantValue.java	57.69%	9 Missing and 2 partials ⚠️
...opensearch/executor/OpenSearchExecutionEngine.java	0.00%	10 Missing ⚠️
...ensearch/sql/data/model/ExprVariantTupleValue.java	84.21%	4 Missing and 2 partials ⚠️
...earch/sql/calcite/utils/OpenSearchTypeFactory.java	20.00%	4 Missing ⚠️
...va/org/opensearch/sql/calcite/utils/PlanUtils.java	55.55%	3 Missing and 1 partial ⚠️
...earch/sql/expression/function/PPLFuncImpTable.java	57.14%	2 Missing and 1 partial ⚠️
...ch/sql/opensearch/data/value/FlatObjectValues.java	92.85%	1 Missing and 2 partials ⚠️

❌ Your project check has failed because the head coverage (63.54%) 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    #5817      +/-   ##
============================================
+ Coverage     63.23%   63.54%   +0.30%     
- Complexity     8823     8997     +174     
============================================
  Files           938      954      +16     
  Lines         40236    40689     +453     
  Branches       4537     4639     +102     
============================================
+ Hits          25445    25854     +409     
+ Misses        13966    13934      -32     
- Partials        825      901      +76     

Flag	Coverage Δ
sql-engine	63.54% <68.46%> (+0.30%)	⬆️

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]

Persistent review updated to latest commit 7b300c1

[github-actions]

Persistent review updated to latest commit daac9c9

[github-actions]

Persistent review updated to latest commit daac9c9

[github-actions]

Persistent review updated to latest commit a44116e

[github-actions]

Persistent review updated to latest commit e24b7d6

[github-actions]

Persistent review updated to latest commit abf3b4e

[github-actions]

Persistent review updated to latest commit b7f0803

[github-actions]

Persistent review updated to latest commit 58074d2

[penghuo]

Two blocking issues, both reproduced against this branch. Details inline.

1. PredicateAnalyzer.flatObjectLeafPredicate's case SEARCH never applies Sarg.nullAs, so leaf != 'a' and leaf != 'b' returns records that never wrote the leaf.
2. In OpenSearchExprValueFactory.parse() the new flat_object branch sits after || content.isArray(), so an array-valued flat_object column never reaches FlatObjectValues.flatten.

Also worth a look, same root cause as the second one — both spellings of a colliding path in one document silently drop a value, and which one survives depends on _source key order:

{"a": {"b": 1}, "a.b": 2}  =>  {a.b: 2}
{"a.b": 2, "a": {"b": 1}}  =>  {a.b: 1}

flattenInto does an unconditional out.put(key, ...), so the later entry wins. Two documents differing only in JSON key order give different query results, and since the index files both values under the one term a.b, a pushed-down attributes.a.b = 1 matches a row that then renders as 2 (and returns nothing at all with pushdown disabled). Representing a colliding path as a collection — which is what the index actually holds, and what this PR already does for array leaves — would keep projection and pushdown in step. constructFlatObject_bothSpellingsCollapseToDottedKey puts each spelling in a separate document, so this case is uncovered.

[penghuo]

Blocking: this branch is unreachable for an array-valued flat_object column, because || content.isArray() above it wins first.

The else if (type.equals(...Nested)) || content.isArray()) branch at line ~221 is evaluated before this one, so a document whose flat_object column is an array goes to parseArray and never reaches FlatObjectValues.flatten.

Run against this branch:

{"flatV": [{"a":{"b":1}}]}  =>  type=ARRAY, value=[{a:"{\"b\":1}"}]
{"flatV": ["a","b"]}       =>  type=ARRAY
{"flatV": []}              =>  NULL

Three consequences for the first case:

• the inner object is stringified as raw JSON text rather than flattened to the dotted key a.b, so attributes.a.b resolves to nothing for that row;
• the column's reported type is ARRAY, not the declared MAP<VARCHAR, VARIANT>, so that row's shape differs from every other row in the same column;
• FlatObjectValues.flatten's documented guard — "null if the value is not an object (a scalar or array ...)" — is dead code, since flatten is never called with an array.

OpenSearch accepts an array at the top level of a flat_object, and the guard in flatten suggests that was the intended handling, so this looks like branch ordering rather than a deliberate choice. Moving the flat_object check above the content.isArray() branch would restore the intent.

No test covers it — every existing flat_object test passes {"flatV": {...}}.

[RyanL1997]

Confirmed and fixed — branch moved above content.isArray() as you suggested

[RyanL1997]

And I didn't restore flatten's guard the way you suggested (array → null), because the index doesn't treat a top-level array as absent — it folds the elements into the same terms, so term flatV.a.b=1 matches [{"a":{"b":1}}] exactly as it matches {"a":{"b":1}}. Null would just swap the 500 for "index answers the filter, projection is null"— the inconsistency you flagged on {"a":{"b":1},"a.b":2}.

[penghuo]

Why add this? VariantNonNull not enought?

[RyanL1997]

Two reasons:

• cast to text. VariantNonNull.cast returns null for a leaf that wasn't written as text. So for a document holding {"code": 503} — a number — the filter where code = '503' matched when pushed down (the index files the term "503") but matched nothing when evaluated on the coordinator with pushdown off. The carrier renders the value as the term the index holds, so both paths agree; there's a yaml case asserting that.
• Array. Calcite's composite variants convert every element with one declared element type, but a flat_object array can hold a number next to a string ([1, "x"]) — each element has to keep its own type.

MapValue is gone, since nothing produces a tuple leaf after the flattening fix. Both reasons are in the class javadoc now.

[penghuo]

Does it support

{
  "a": {"b": 1},
  "a.b": 2
}

[RyanL1997]

Fixed, it now supported:

{"a": {"b": 1}, "a.b": 2}  ->  a.b: [1, 2]

Both values should be supported, because OpenSearch supports them: the same document on a plain mapped object field also indexes both, and term a.b=1 / term a.b=2 each match. Returning one was the bug.

Same fix covers a top-level array and an object inside an array. Tests added for all of them.

[penghuo]

To be clear on limitations.

[RyanL1997]

re-wrote the part with to lists of what we support and what we not support. (https://github.com/RyanL1997/sql/blob/feat/flat-object/docs/user/ppl/general/datatypes.md#query-struct-data-types)

[penghuo]

what is the value of "a.b"?

[RyanL1997]

if you are referring to the example of

{"a": {"b": 1}} and {"a.b": 1}

The value should be one - similar to the above comment I have re-wrote the above doc paragraph and please re-take a look.

[penghuo]

Can validate in analyze()?

[RyanL1997]

Done — moved into CalciteRelNodeVisitor.analyze.

[penghuo]

It is not a flat_object, it is close to ExprTupleValue, right? the difference, it expect return VariantValue? The n we should define in core module instead in opensearch module.

[RyanL1997]

You're right. Moved and renamed:

• OpenSearchExprFlatObjectValue → core/.../data/model/ExprVariantTupleValue — an ExprTupleValue that hands its values to Calcite as variants, which is all it does
• OpenSearchVariantValue → core/.../data/model/ExprVariantValue

[github-actions]

Persistent review updated to latest commit c20db71

[github-actions]

Persistent review updated to latest commit 506496e

[github-actions]

Persistent review updated to latest commit 935b958

[github-actions]

Persistent review updated to latest commit 99d3400

[github-actions]

Persistent review updated to latest commit ce07556

[github-actions]

Persistent review updated to latest commit 34340bb

[github-actions]

Persistent review updated to latest commit 00fa3f0

[github-actions]

Persistent review updated to latest commit 9ce89fd

[github-actions]

Persistent review updated to latest commit d2aa48f

[penghuo]

Review scoped to ExprVariantValue.Array only; comments inline.

Short version: the existence of a custom Array carrier looks justified to me — Calcite's own ARRAY variant is homogeneous by construction, so a heterogeneous flat_object array genuinely can't go through it. The concern is that cast() and item() return null unconditionally, which makes array-leaf behaviour depend on whether pushdown is enabled, and both failure modes are silent (no error, just missing rows).

[penghuo]

cast() returning null for every target type makes a filter on an array leaf answer differently depending on whether pushdown is on, which contradicts the invariant this PR states ("a filter evaluated on the coordinator returns the same rows as the same filter pushed down").

With plugins.calcite.pushdown.enabled=false, where attributes.tags = 'a' lowers to a comparison over cast(<variant> AS VARCHAR). For an Array that cast is null, so the row is dropped — while the pushdown-on path (asserted in 1604.yml) returns ["tags"]. The merged-path case the description advertises ({"a":{"b":1},"a.b":2} → [1,2]) has the same problem: where attributes.a.b = 1 matches with pushdown on and matches nothing with it off.

The "same answer with pushdown off" section of 1604.yml only exercises scalar leaves, so CI stays green. Adding an array leaf there should reproduce it.

Worth flagging that the fix is probably not "implement cast": a term query on an array field in OpenSearch has ANY semantics (matches if any element equals the term), and a scalar cast to a single VARCHAR can't express ANY — rendering the list as "[a, b]" would just introduce a third inconsistent answer. Two options that do hold:

1. give array equality a real multi-value/ANY operator, so the coordinator-side and pushed-down paths agree; or
2. have FlatObjectScopeValidator reject predicates on array leaves when pushdown is unavailable — an explicit error is better than silently returning fewer rows.

Either way, could the bare return null carry a comment stating the invariant? As written there's no way for the next reader to tell whether this is a deliberate decision or a stub.

[penghuo]

Same shape of concern as cast above: item() returning null unconditionally means attributes.tags[0] silently yields null rather than the element (or an error). If indexing into an array leaf isn't supported, it would be better to say so explicitly — either reject it in FlatObjectScopeValidator or document it here.

Stepping back on the class as a whole: Array extends VariantSqlValue but answers null from every variant operation, so it can't stand in for one (Effective Java Item 20). To be clear, having read the Calcite side I agree a custom carrier is necessary — VariantNonNull's ARRAY ctor applies a single getTypeArgument(0) to every element together with a per-tag assert value instanceof X, so a heterogeneous array (4 next to "n/a") can't be represented, and ARRAY<VARIANT> is degenerate because VARIANT-tagged elements fall through cast to null anyway. The open question is only whether this should participate in the variant type system while supporting none of its operations, or be an explicitly opaque carrier that the boundary unwraps — which is what unwrapAll() already does.

[RyanL1997]

Closing due to #5830 - Instead of introducing flat object as a VARIANT type it introduce it as a STRING type. Here are the reasons from internal discussions with @penghuo:

---

Why text rather than a type per leaf

spath already presents a field extracted from a JSON string as text, with a path holding several values reading as their JSON. A flat_object leaf is the same shape, so it reads the same way — one rule for users, and the engine's existing handling of text with nothing new to teach it.

VARIANT would keep the type a leaf was written with, which only post-processing reads, and post-processing needs a script either way since a flat_object has no doc values. So it adds a value type the engine has to learn everywhere it passes, and buys nothing the index can act on.