Support multi-value keyword fields in composite engine

[Bukhtawar]

Description

Adds opt-in multi-value (array) support for keyword fields in the composite Lucene+Parquet engine, stored as Arrow/Parquet LIST columns.

Cardinality is declared per field via a new multi_value mapping parameter:

PUT /logs
{
  "mappings": { "properties": { "tags": { "type": "keyword", "multi_value": true } } }
}

{"tags": ["beta","alpha","beta"]} now indexes and reads back intact, order and duplicates preserved.

Why opt-in rather than automatic

OpenSearch mappings never carry cardinality — an array is a property of an individual document — but a Parquet column's type is fixed for the whole file. The setting bridges that gap. Alternatives considered:

• Always LIST: adds a 4-byte offset per row per column, changes the on-disk schema of every existing composite index, and makes every scalar read unwrap a list.
• Auto-promote scalar → LIST on first array: not expressible. The native writer's schema freezes once initialized (VSRManager.isSchemaMutable() goes false after the first flush), so promotion would need an in-flight file rewrite.

Opt-in keeps cost and blast radius at zero for undeclared fields, which keep their scalar column and still reject a second value (with an error naming the parameter). A mapping parameter rather than an index setting because cardinality is a property of the field: the declaration travels with the field definition, the mapping is self-describing for any consumer that reads it (the read-side Calcite schema builder reads the property straight from the mapping — no settings mirror to keep in sync), and "declared field doesn't exist" misconfiguration is structurally impossible. The parameter is not updateable (registerConflictCheck) because the column type is baked into every file the index has written. MappedFieldType gains experimental isMultiValued()/setMultiValued() to carry the flag to the document-input layer.

Implementation

Write path. ParquetDocumentInput accumulates values rather than throwing on the second one — DocumentParser already calls addField once per array element, so no mapper changes were needed. Order and duplicates are preserved because _source is derived from these columns. The list write protocol (startNewValue / setSafe / endValue, null list for an absent field) lives once in ParquetField, so adding another field type means overriding addToVector + supportsMultiValue().

Read path. The Calcite schema types declared fields as ARRAY<element>; ArrowCalciteTypes.toArrowField carries the element type (an Arrow list holds its child on the Field, not the ArrowType); ARRAY joins the DataFusion scan capability but deliberately not filter/sort/aggregate, where array semantics are undefined; and ArrowValues.toSourceValue renders list columns so get-by-id no longer silently drops the field.

Testing

• Unit: document-input accumulation and rejection; VSR list writes (multi-value, single value, absent, empty); reconcileSchema child preservation; Calcite ARRAY typing and element-type propagation; Rust column-path/element-type resolution.
• Rust merge: sorted and unsorted merges preserving per-row values; multi-batch cursors under deferred column decode (the adversarial case — value counts unrelated to row counts in every batch); and a clean Unsupported sort column type error when a LIST column is used as a sort key.
• Indexing lifecycle & edge cases (MultiValueFieldDurabilityIT, internalClusterTest, 9 tests): refresh, flush, force-merge, segment replication to a peer (asserting the replica's catalog converges on the primary's parquet files), node-restart recovery, peer recovery while indexing continues (a BackgroundIndexer subclass emitting multi-valued docs, so a generation can ship while the primary's VSR is mid-list), and cross-format partial failure — a tags element over Lucene's MAX_TERM_LENGTH fails only in the secondary, after parquet accepted the row and advanced its list offsets, so CompositeWriter's rollback must rewind a partially written LIST cell; the test asserts the doc lands in neither format and that writes resume correctly. Verified non-vacuous via the run logs ("Failed to add document in secondary format [lucene], rolling back" + Lucene's "immense term in field="tags""). Each stage re-reads every document and requires values byte-identical, in order, duplicates intact. Also pins: null elements inside an array are dropped (["a",null,"b"] stores as ["a","b"] — matches Lucene, but the alternative is defensible so it is pinned); a 50k-element array in one document (VSR rotation is row-count driven, so one pathological array grows the child vector without rotating); and a multi_value field added by mapping update, the only path reaching VSRManager.reconcileSchema for a LIST column — confirmed non-vacuous by mutation (restoring the old field rebuild makes it fail with "Lists have one child Field. Found: none"). Also bulk with interleaved valid/invalid documents — a rejected item first, between, and last — since CompositeWriter rolls back to the composite's running acceptedRows and a mistake there would corrupt the failing document's neighbours; asserts all valid neighbours keep byte-identical arrays and writes resume.
• Integration (MultiValueFieldIT): ingest; projection on both read paths as a control pair (ListingTable vs indexed); array_length over a LIST column; get-by-id _source reconstruction; force-merge round-trip asserting every document keeps exactly its own values; and the two rejection paths.
• precommit passes on all four touched modules. No regressions in ListAggregateMultiShardIT, TwoShardScalarIT, SortCommandIT, ArrayFunctionIT.

Known limitations / follow-ups

• keyword only. text, ip, numerics, dates, boolean remain scalar-only; the field-type layer is ready for them.
• "tags": [] reads back as null, not an empty list. An explicit empty array yields zero addField calls, so the writer never sees the field — indistinguishable from absent. Tested and documented; worth deciding whether to preserve the distinction.
• A multi_value field cannot be used as index.sort.field — rejected at index creation with an error naming the field (a multi-valued cell has no single value to sort on; matches Lucene's own rejection). The native merge's clean error remains as defense in depth, both covered by tests.
• where tags = 'x' means CONTAINS (Lucene term-query parity) — this PR carries the engine half (ScalarFunction.ARRAY_CONTAINS, Substrait mapping to DataFusion's array_has, FieldStorageResolver typing multi_value fields as ARRAY, filter capabilities); the analyzer half is Equality on an ARRAY-typed field means CONTAINS sql#5694 (rewrites =/!= on [ARRAY<T>, scalar] to ARRAY_CONTAINS/NOT(ARRAY_CONTAINS)). Verified green end-to-end against a local unified-query build; the IT is @AwaitsFix until the SQL PR ships in the published snapshot. Other pinned semantics: mvfind(tags,'x') >= 0 and array_length(tags) > n filters work; sort tags orders lexicographically by elements (nulls first); stats … by tags groups by the whole array value (per-element bucketing needs unnest, future work).
• Remote-store restore and replica promotion with a multi_value field are not covered (their base ITs use flat fields).
• No performance measurement. ListVsScalarBenchmark exists for exactly this and hasn't been run; the opt-in design confines any cost to declared fields.
• Cross-shard reduce over a LIST column is untested (the REST IT is single-shard so expected values stay exact; the durability IT covers a 2-node replica pair but not a multi-shard scatter/reduce).

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 Code Suggestions ✨

Latest suggestions up to 5559906

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Verify semantics change for all callers Changing getFieldCount to sum valueCount() alters semantics for all callers, but the getFinalInput assertions immediately below assert getFieldCount(...) == 1 for id/version/seqNo/primary-term. If any of those metadata fields ever became multi-valued (or a bug caused duplicate registrations to accumulate), those assertions would still fire — which is fine — but the data-stream @timestamp check will now reject a multi-valued timestamp declaration as "multiple values" even though only one entry exists. Verify the @timestamp validation path still behaves correctly, or make the semantics explicit per-caller. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/writer/ParquetDocumentInput.java [101-108] +@Override +public long getFieldCount(String fieldName) { + // Counts values, not entries: a multi-valued field is one entry holding N values, and + // callers (single-value assertions below, the data-stream @timestamp check) mean values. + return collectedFields.stream() + .filter(fvp -> fvp.getFieldType().name().equals(fieldName)) + .mapToLong(FieldValuePair::valueCount) + .sum(); +} - Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid verification concern about semantics change affecting @timestamp validation, but the improved_code is identical to existing_code and only asks the author to verify — limiting its actionable impact.	Low
	Avoid null-hostile List.of wrapping List.of(parseValue) throws NPE when parseValue is non-null but not a List and happens to be a null-wrapping edge case; more importantly, when a caller passes a singular non-null value it should still be wrapped. Consider using Collections.singletonList(parseValue) or a mutable wrapper to avoid List.of's null-hostile contract in case a null slips through the earlier guard in the future. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/fields/ParquetField.java [137-148] -List<?> values = parseValue instanceof List<?> list ? list : List.of(parseValue); +List<?> values = parseValue instanceof List<?> list ? list : java.util.Collections.singletonList(parseValue); int start = listVector.startNewValue(row); FieldVector dataVector = listVector.getDataVector(); for (int i = 0; i < values.size(); i++) { Object value = values.get(i); if (value == null) { dataVector.setNull(start + i); } else { addToVector(dataVector, start + i, value); } } listVector.endValue(row, values.size()); Suggestion importance[1-10]: 2 __ Why: parseValue is already null-checked at the top of writeList, so List.of(parseValue) cannot be called with null. The suggestion is defensive against a hypothetical future change and has minimal current impact.	Low
Possible issue	Preserve null-vs-empty list distinction The list-branch runs before the null-check based on vec.isNull(idx) semantics for lists — but for a ListVector, an unset (null) list still has valid offsets. The current code will return an empty List for a null list rather than null, losing the distinction between an absent field (null list) and an empty list ([]). Return null (or check listVector.isNull(idx)) before reading offsets. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ArrowValues.java [91-100] if (vec instanceof ListVector listVector && vec instanceof MapVector == false) { + if (listVector.isNull(idx)) { + return null; + } FieldVector data = listVector.getDataVector(); int start = listVector.getOffsetBuffer().getInt((long) idx * ListVector.OFFSET_WIDTH); int end = listVector.getOffsetBuffer().getInt((long) (idx + 1) * ListVector.OFFSET_WIDTH); List<Object> values = new ArrayList<>(Math.max(0, end - start)); for (int i = start; i < end; i++) { values.add(toSourceValue(data, i)); } return values; } Suggestion importance[1-10]: 3 __ Why: The outer toSourceValue method already checks vec.isNull(idx) at line 76 and returns null, so a null list is handled before reaching this branch. The suggestion is largely redundant, though an extra defensive check is not harmful.	Low

---

Previous suggestions

Suggestions up to commit 827515b

Category	Suggestion	Impact
General	Avoid unconditional schema build for validation Previously, when hasParquetSettings was false the validator returned early and never called ArrowSchemaBuilder.getSchema. Now it always builds the schema on any parquet index — a behavior change that could regress index creation performance/behavior for indices that have no parquet field-level settings. Confirm this is intentional or gate schema-building on hasParquetSettings while keeping sort-field validation unconditional. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetIndexCreationValidator.java [43-55] if (!isParquetIndex) { return; } validateSortFieldsAreSingleValued(mapperService, indexSettings); -// Building the schema validates the mapping's `multi_value` declarations: getSchema throws -// for a field whose type has no list support, turning what would otherwise be a -// per-document indexing failure into an immediate error at creation time. -Schema schema = ArrowSchemaBuilder.getSchema(mapperService); if (hasParquetSettings) { + // Building the schema validates the mapping's `multi_value` declarations: getSchema throws + // for a field whose type has no list support, turning what would otherwise be a + // per-document indexing failure into an immediate error at creation time. + Schema schema = ArrowSchemaBuilder.getSchema(mapperService); ParquetSettings.validateFieldConfigurations(fieldEncodings, fieldCompressions, fieldBloomFilterEnabled, schema); } Suggestion importance[1-10]: 6 __ Why: The suggestion misses that the schema-build is now intentionally used to validate multi_value declarations at creation time (per the comment). Reverting to conditional would lose that new validation. However, the reviewer correctly flags a behavior change worth confirming.	Low
	Use offset accessors instead of raw buffer The offset buffer read multiplies idx by ListVector.OFFSET_WIDTH (4 bytes) — correct for a standard ListVector but breaks if a LargeListVector is ever routed through this code path (offsets are 8 bytes there). Consider using listVector.getElementStartIndex(idx) / getElementEndIndex(idx) (or listVector.getObject) to avoid the raw offset arithmetic and stay compatible with variants. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ArrowValues.java [91-100] if (vec instanceof ListVector listVector && vec instanceof MapVector == false) { FieldVector data = listVector.getDataVector(); - int start = listVector.getOffsetBuffer().getInt((long) idx * ListVector.OFFSET_WIDTH); - int end = listVector.getOffsetBuffer().getInt((long) (idx + 1) * ListVector.OFFSET_WIDTH); + int start = listVector.getElementStartIndex(idx); + int end = listVector.getElementEndIndex(idx); List<Object> values = new ArrayList<>(Math.max(0, end - start)); for (int i = start; i < end; i++) { values.add(toSourceValue(data, i)); } return values; } Suggestion importance[1-10]: 5 __ Why: Using getElementStartIndex/getElementEndIndex is more idiomatic and slightly more robust, though the current code is limited to ListVector (checked via instanceof) so LargeListVector is not actually a concern here. Minor readability improvement.	Low
	Handle empty-list case explicitly List.of(parseValue) throws NPE if parseValue is null — that is already guarded above — but it also disallows null elements if a caller ever passes a singleton list. More importantly, this branch is not reached for empty-list JSON (which never calls addField); however if a caller supplies an empty List here, startNewValue/endValue(row, 0) produces an empty (non-null) list, which is inconsistent with the "absent = null" contract documented elsewhere. Consider handling values.isEmpty() explicitly to make the intended semantics unambiguous. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/fields/ParquetField.java [131-149] private void writeList(MappedFieldType fieldType, ManagedVSR managedVSR, ListVector listVector, Object parseValue) { int row = managedVSR.getRowCount(); if (parseValue == null) { listVector.setNull(row); return; } List<?> values = parseValue instanceof List<?> list ? list : List.of(parseValue); + if (values.isEmpty()) { + // An explicitly-empty list is stored as a zero-length, non-null list — preserving the + // distinction between "tags": [] and an absent field. + listVector.startNewValue(row); + listVector.endValue(row, 0); + return; + } int start = listVector.startNewValue(row); FieldVector dataVector = listVector.getDataVector(); for (int i = 0; i < values.size(); i++) { Object value = values.get(i); if (value == null) { dataVector.setNull(start + i); } else { addToVector(dataVector, start + i, value); } } listVector.endValue(row, values.size()); } Suggestion importance[1-10]: 3 __ Why: The existing code already correctly handles an empty list via startNewValue/endValue(row, 0). The suggested change is essentially a stylistic refactor that separates the empty case but doesn't fix a bug — the current behavior already matches the documented "empty list is zero-length non-null" contract.	Low
	Assert multi-value invariant on companion field The raw-value companion field is added regardless of whether the parquet field supports multi-value storage. If multiValue is true but parquetField.supportsMultiValue() is false, this would have already thrown earlier, but if a mapper type ever supports scalar storage but not list storage, toArrowField(name, true) would throw here inconsistently. Consider guarding this call or asserting the invariant explicitly for clarity. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/fields/ArrowSchemaBuilder.java [96-102] if (mapper instanceof KeywordFieldMapper keywordFieldMapper) { if (!documentMapper.mappers().isMultiField(mapper.name()) && keywordFieldMapper.getRawValueFieldType() != null) { KeywordFieldMapper.KeywordFieldType rawValueField = keywordFieldMapper.getRawValueFieldType(); // The raw-value companion holds the pre-normalization source for derived source, so // it must mirror the parent's cardinality or source reconstruction would lose values. + assert multiValue == false || parquetField.supportsMultiValue() : "parent multi_value check should have failed already"; fields.add(parquetField.toArrowField(rawValueField.name(), multiValue)); } } Suggestion importance[1-10]: 2 __ Why: Adding an assertion here is a minor defensive change; the invariant is already enforced earlier in the loop where multiValue && !supportsMultiValue() throws, so the assertion is largely redundant.	Low

Suggestions up to commit 8cba589

Category	Suggestion	Impact
General	Avoid NPE on null scalar in list write List.of(parseValue) throws NPE if parseValue is a scalar null. The caller already handles parseValue == null above (writing a null list), but if a non-list scalar-null ever slips through (e.g. from an alternate call path), this would fail obscurely. Use Collections.singletonList(parseValue) or wrap defensively to preserve the intended "null element" semantics. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/fields/ParquetField.java [137-148] -List<?> values = parseValue instanceof List<?> list ? list : List.of(parseValue); +List<?> values = parseValue instanceof List<?> list ? list : java.util.Collections.singletonList(parseValue); int start = listVector.startNewValue(row); FieldVector dataVector = listVector.getDataVector(); for (int i = 0; i < values.size(); i++) { Object value = values.get(i); if (value == null) { dataVector.setNull(start + i); } else { addToVector(dataVector, start + i, value); } } listVector.endValue(row, values.size()); Suggestion importance[1-10]: 3 __ Why: Minor defensive improvement. The caller path already handles null parseValue, so the risk is hypothetical, but using Collections.singletonList would be marginally safer.	Low
	Consider upgrade impact of stricter validation Previously the early-return skipped schema building when !hasParquetSettings. Now ArrowSchemaBuilder.getSchema(mapperService) is always invoked for a parquet index, which may throw at index creation for existing indices that previously succeeded (e.g. mappings with multi_value on unsupported types). This is intentional per the comment, but ensure it does not break existing indices during upgrade — consider only running the multi_value validation without the full schema build if no parquet field settings exist and no multi_value declarations are present. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetIndexCreationValidator.java [43-55] if (!isParquetIndex) { return; } validateSortFieldsAreSingleValued(mapperService, indexSettings); -// Building the schema validates the mapping's `multi_value` declarations: getSchema throws -// for a field whose type has no list support, turning what would otherwise be a -// per-document indexing failure into an immediate error at creation time. Schema schema = ArrowSchemaBuilder.getSchema(mapperService); if (hasParquetSettings) { ParquetSettings.validateFieldConfigurations(fieldEncodings, fieldCompressions, fieldBloomFilterEnabled, schema); } Suggestion importance[1-10]: 3 __ Why: Raises a plausible upgrade concern but only asks to "consider" it; improved_code merely removes a comment and is effectively equivalent to existing_code.	Low
	Verify collected-entries count semantics The collectedFields list is only appended to when existing == null. For a multi-valued field that already has a pair, existing.addValue(value) mutates the pair in place — which is correct — but this means getFinalInput().size() returns the number of distinct fields, not the total value count. The test testMultiValueFieldCountIsValueCountNotEntryCount asserts getFinalInput().size() == 5, which would only hold if the multi-valued pair were added multiple times. Verify the intended contract: either the test expectation is wrong (should be 4: id, seqno, primary_term, version, tags = 5 entries; but tags added 3 times as separate entries would be 7) or collectedFields should append on every value. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/writer/ParquetDocumentInput.java [68-77] +if (existing.isMultiValued() == false) { + throw new MapperParsingException( + "Cannot accept multiple values for field: [" + + fieldType.name() + + "] of type: [" + + fieldType.typeName() + + "]. Set [multi_value: true] on the field mapping to store multiple values." + ); +} +existing.addValue(value); - Suggestion importance[1-10]: 2 __ Why: The suggestion is speculative and asks the author to "verify" behavior without proposing a concrete change; improved_code equals existing_code. The test expectation of size 5 is plausible (4 metadata entries + 1 multi-valued tags entry).	Low
	Document assumption on offset width The offset buffer is being read using getInt with the raw byte offset idx * OFFSET_WIDTH, which is correct, but the initial null check vec.isNull(idx) at the top of the method already handles null lists. For an empty (non-null) list, end - start == 0 and an empty list is returned — good. However, consider that ListVector.OFFSET_WIDTH is 4 bytes, so if the vector uses a LargeListVector (8-byte offsets) this cast would silently read wrong offsets. Guard against unexpected list variants explicitly. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ArrowValues.java [91-100] if (vec instanceof ListVector listVector && vec instanceof MapVector == false) { + List<Object> values = new ArrayList<>(); FieldVector data = listVector.getDataVector(); int start = listVector.getOffsetBuffer().getInt((long) idx * ListVector.OFFSET_WIDTH); int end = listVector.getOffsetBuffer().getInt((long) (idx + 1) * ListVector.OFFSET_WIDTH); - List<Object> values = new ArrayList<>(Math.max(0, end - start)); for (int i = start; i < end; i++) { values.add(toSourceValue(data, i)); } return values; } Suggestion importance[1-10]: 2 __ Why: improved_code is essentially identical to existing_code (just reordered) and does not add the LargeListVector guard the description mentions. Low impact.	Low

Suggestions up to commit 3353037

Category	Suggestion	Impact
Possible issue	Use fixed element segment name Using child.name() in the path is fragile: the arrow-rs parquet writer uses fixed segment names ("list" / "element") regardless of the arrow child field's declared name. If a caller constructs a List with a child field named something other than "element", the computed path won't match what the writer emits, silently reverting to defaults. Hard-code "element" (as LIST_ELEMENT_NAME does on the Java side) instead of reading it from the child field. sandbox/plugins/parquet-data-format/src/main/rust/src/writer_properties_builder.rs [85-95] fn column_path_for(field: &arrow::datatypes::Field) -> parquet::schema::types::ColumnPath { use arrow::datatypes::DataType::*; let mut parts = vec![field.name().clone()]; let mut current = field.data_type(); while let List(child) | LargeList(child) | FixedSizeList(child, _) = current { parts.push("list".to_string()); - parts.push(child.name().clone()); + // arrow-rs's parquet writer emits the fixed segment name "element" for list children, + // irrespective of the arrow child field's declared name. + parts.push("element".to_string()); current = child.data_type(); } parquet::schema::types::ColumnPath::new(parts) } Suggestion importance[1-10]: 7 __ Why: This is a potentially real bug: arrow-rs's parquet writer uses fixed segment names ("list"/"element") regardless of the arrow child field's declared name. If a caller uses a different child name, the column path would not match and per-column settings would silently no-op — exactly the bug this PR is fixing for the top-level case.	Medium
General	Avoid List.of null-rejection for scalars List.of(parseValue) throws NPE if parseValue is null, but that case is already handled above. However, List.of(parseValue) rejects null elements — so if a caller passes a non-List parseValue that happens to be null-checked elsewhere but arrives as null here in some edge case, this would NPE. More critically, the branch is entered for FieldValuePair which for multi-valued fields always wraps values in a List (via FieldValuePair.multiValued), but for single-valued declared-multi_value pairs it also wraps them; still, if any caller path delivers a non-List, non-null scalar, this is safe. Consider using Collections.singletonList for consistency or asserting the expected shape. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/fields/ParquetField.java [131-149] private void writeList(MappedFieldType fieldType, ManagedVSR managedVSR, ListVector listVector, Object parseValue) { int row = managedVSR.getRowCount(); if (parseValue == null) { listVector.setNull(row); return; } - List<?> values = parseValue instanceof List<?> list ? list : List.of(parseValue); + List<?> values = parseValue instanceof List<?> list ? list : java.util.Collections.singletonList(parseValue); int start = listVector.startNewValue(row); FieldVector dataVector = listVector.getDataVector(); for (int i = 0; i < values.size(); i++) { Object value = values.get(i); if (value == null) { dataVector.setNull(start + i); } else { addToVector(dataVector, start + i, value); } } listVector.endValue(row, values.size()); } Suggestion importance[1-10]: 4 __ Why: Using Collections.singletonList instead of List.of would tolerate null scalars, but the suggestion itself notes that the null case is handled above. It's a minor defensive improvement with limited practical impact.	Low
	Verify null-vs-absent list semantics The early null check if (vec == null || vec.isNull(idx)) return null; is executed before this list handling, but the offset buffer read uses (long) idx * ListVector.OFFSET_WIDTH — ensure the multiplication is done on a long to avoid overflow for large idx. More importantly, the null check at the top returns null for a null list, but the caller (toSourceMap) then drops the field entirely, losing the distinction between an absent field and an explicit null. Consider whether returning an empty list vs null for the "absent" case aligns with the documented semantics. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ArrowValues.java [91-100] if (vec instanceof ListVector listVector && vec instanceof MapVector == false) { FieldVector data = listVector.getDataVector(); - int start = listVector.getOffsetBuffer().getInt((long) idx * ListVector.OFFSET_WIDTH); - int end = listVector.getOffsetBuffer().getInt((long) (idx + 1) * ListVector.OFFSET_WIDTH); + long offsetBase = (long) idx * ListVector.OFFSET_WIDTH; + int start = listVector.getOffsetBuffer().getInt(offsetBase); + int end = listVector.getOffsetBuffer().getInt(offsetBase + ListVector.OFFSET_WIDTH); List<Object> values = new ArrayList<>(Math.max(0, end - start)); for (int i = start; i < end; i++) { values.add(toSourceValue(data, i)); } return values; } Suggestion importance[1-10]: 3 __ Why: The overflow concern is minor since idx is an int and the multiplication is already cast to long. The null-vs-absent semantics observation is a reasonable design question but does not identify a clear bug, and the improved code is essentially equivalent to the original.	Low
	Assert schema-root correspondence invariant The comment claims a 1:1 field↔root order between the arrow schema and parquet leaves because arrow_schema is derived from parquet_to_arrow_schema. However, this function accepts an arbitrary arrow_schema parameter, and callers could pass a projected/reordered schema. If root_idx from the arrow schema does not match parquet's root indexing, the wrong leaves would be selected. Add a defensive assertion or document the caller contract explicitly. sandbox/plugins/analytics-backend-datafusion/rust/src/cache/page_index/column_schema_resolver.rs [97-106] if let Some((root_idx, field)) = arrow_schema.fields().find(name) { if field.data_type().is_nested() { + // Caller contract: `arrow_schema` must be derived from this file's footer + // (parquet_to_arrow_schema), so arrow root index == parquet root index. + debug_assert!( + root_idx < parquet_schema.root_schema().get_fields().len(), + "arrow root index {} out of range for parquet schema (name={})", + root_idx, name + ); for leaf in 0..parquet_schema.num_columns() { if parquet_schema.get_column_root_idx(leaf) == root_idx { set.insert(leaf); } } continue; } } Suggestion importance[1-10]: 3 __ Why: Adding a debug_assert documents the caller contract but does not change behavior. The suggestion is minor and mostly documentation-oriented.	Low

Suggestions up to commit cdd8f96

Category	Suggestion	Impact
General	Reconsider changing getFieldCount semantics Changing getFieldCount semantics from "entry count" to "value count" may break callers that expected the previous behavior (counting occurrences of addField). In particular, the assert getFieldCount(IdFieldMapper.NAME) == 1 etc. now depend on metadata fields being single-valued — which is true today, but any future metadata field mapped as multi_value would silently satisfy the assertion with N values instead of failing. Consider keeping a separate getValueCount method for the value-count semantics and leaving getFieldCount returning entries, to avoid subtle behavior changes to existing callers. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/writer/ParquetDocumentInput.java [101-108] @Override public long getFieldCount(String fieldName) { - // Counts values, not entries: a multi-valued field is one entry holding N values, and - // callers (single-value assertions below, the data-stream @timestamp check) mean values. return collectedFields.stream() .filter(fvp -> fvp.getFieldType().name().equals(fieldName)) .mapToLong(FieldValuePair::valueCount) .sum(); } Suggestion importance[1-10]: 5 __ Why: Valid concern about semantic changes to getFieldCount, but the improved_code is essentially identical to the existing_code (only a comment is removed), so the suggestion doesn't actually propose a concrete alternative implementation.	Low
	Verify NOT capability registration for ARRAY Registering ScalarFunction.NOT against FieldType.ARRAY is misleading — NOT operates on a boolean result, not on an ARRAY value. The capability lookup will consult the field types of the NOT's argument (an ARRAY_CONTAINS call returning boolean), so declaring NOT for ARRAY fields is only meaningful if the capability system evaluates NOT against the underlying column type of its inner expression. Verify this matches the filter rule's actual lookup semantics; otherwise NOT(ARRAY_CONTAINS(...)) (for !=) may not be accepted. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionAnalyticsBackendPlugin.java [617-618] +caps.add(new FilterCapability.Standard(ScalarFunction.ARRAY_CONTAINS, Set.of(FieldType.ARRAY), formats)); +caps.add(new FilterCapability.Standard(ScalarFunction.NOT, Set.of(FieldType.ARRAY), formats)); - Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid question about capability semantics but only asks the author to verify, and improved_code is identical to existing_code, offering no concrete fix.	Low
	Use singletonList for scalar wrapping List.of(parseValue) throws NullPointerException if parseValue is non-null but a single scalar happens to be null-wrapped or if the caller ever passes a null element wrapped as scalar. Also, using List.of disallows nulls in the list itself; if the incoming parseValue is a List containing nulls that is fine (it's not created by List.of), but the singleton branch would blow up on null. Since null parseValue is already handled above, this is safe, but consider Collections.singletonList(parseValue) for consistency and clarity. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/fields/ParquetField.java [137] -List<?> values = parseValue instanceof List<?> list ? list : List.of(parseValue); -int start = listVector.startNewValue(row); -FieldVector dataVector = listVector.getDataVector(); -for (int i = 0; i < values.size(); i++) { - Object value = values.get(i); - if (value == null) { - dataVector.setNull(start + i); - } else { - addToVector(dataVector, start + i, value); - } -} -listVector.endValue(row, values.size()); +List<?> values = parseValue instanceof List<?> list ? list : java.util.Collections.singletonList(parseValue); Suggestion importance[1-10]: 2 __ Why: Since null parseValue is already handled earlier, List.of(parseValue) is safe here. The suggestion acknowledges this and offers only a marginal stylistic change.	Low
Possible issue	Guard null list before reading offsets The offset buffer index must be multiplied by OFFSET_WIDTH in bytes, but getInt already takes a byte offset. Verify units — using (long) idx * ListVector.OFFSET_WIDTH is correct only if OFFSET_WIDTH is 4 (bytes per int32 offset). Also, ensure the list is validated as non-null via listVector.isNull(idx) before reading offsets, otherwise a null list may yield stale start/end values and produce spurious elements. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ArrowValues.java [91-100] if (vec instanceof ListVector listVector && vec instanceof MapVector == false) { + if (listVector.isNull(idx)) { + return null; + } FieldVector data = listVector.getDataVector(); int start = listVector.getOffsetBuffer().getInt((long) idx * ListVector.OFFSET_WIDTH); int end = listVector.getOffsetBuffer().getInt((long) (idx + 1) * ListVector.OFFSET_WIDTH); List<Object> values = new ArrayList<>(Math.max(0, end - start)); for (int i = start; i < end; i++) { values.add(toSourceValue(data, i)); } return values; } Suggestion importance[1-10]: 3 __ Why: The null check vec.isNull(idx) is already performed at the top of toSourceValue (line 76), so this additional guard is redundant. The suggestion is not incorrect but adds minimal value.	Low

Suggestions up to commit c53d71f

Category	Suggestion	Impact
Possible issue	Use canonical element segment name The child field's name is used as-is for the leaf segment, but arrow-rs's parquet writer emits a fixed segment name (element) regardless of the arrow child field's name. If the arrow child is not literally named "element" (which can happen with user-constructed schemas), the resulting ColumnPath will not match the actual parquet leaf and per-column settings will silently no-op. Use the canonical "element" segment to match the writer's output. sandbox/plugins/parquet-data-format/src/main/rust/src/writer_properties_builder.rs [85-95] fn column_path_for(field: &arrow::datatypes::Field) -> parquet::schema::types::ColumnPath { use arrow::datatypes::DataType::*; let mut parts = vec![field.name().clone()]; let mut current = field.data_type(); while let List(child) | LargeList(child) | FixedSizeList(child, _) = current { parts.push("list".to_string()); - parts.push(child.name().clone()); + parts.push("element".to_string()); current = child.data_type(); } parquet::schema::types::ColumnPath::new(parts) } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly notes that arrow-rs's parquet writer uses a fixed element segment name, and using the arrow child's name (which could differ) would silently break per-column settings. The PR's own code and tests use "element" consistently, so hardcoding it defensively is reasonable, though the current code works for the typical case.	Low
General	Use documented ListVector element index APIs ListVector.getOffsetBuffer().getInt(index) takes a byte offset, not an element index. Multiplying idx * OFFSET_WIDTH gives the correct byte offset, but the cast should ensure the multiplication happens in long before being passed; also verify OFFSET_WIDTH is the byte-width constant used consistently. If OFFSET_WIDTH is in bytes this is correct; otherwise offsets will be read from wrong positions and return corrupted list ranges. Prefer using listVector.getElementStartIndex(idx) / getElementEndIndex(idx) which are the documented APIs and avoid this ambiguity. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ArrowValues.java [91-100] if (vec instanceof ListVector listVector && vec instanceof MapVector == false) { FieldVector data = listVector.getDataVector(); - int start = listVector.getOffsetBuffer().getInt((long) idx * ListVector.OFFSET_WIDTH); - int end = listVector.getOffsetBuffer().getInt((long) (idx + 1) * ListVector.OFFSET_WIDTH); + int start = listVector.getElementStartIndex(idx); + int end = listVector.getElementEndIndex(idx); List<Object> values = new ArrayList<>(Math.max(0, end - start)); for (int i = start; i < end; i++) { values.add(toSourceValue(data, i)); } return values; } Suggestion importance[1-10]: 5 __ Why: Using getElementStartIndex/getElementEndIndex is cleaner and more idiomatic than manually computing offsets with OFFSET_WIDTH. The existing code is functionally correct (OFFSET_WIDTH is a byte width constant in Arrow), so this is a readability/maintainability improvement rather than a bug fix.	Low
	Prevent external mutation of accumulated values Returning the internal mutable values list directly exposes the pair's backing storage — a consumer could mutate it and corrupt the accumulated state, or continue to see updates if addValue is called after getValue. Return an unmodifiable view (or a defensive copy) to preserve the "values are the source of truth" invariant documented in the class. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/writer/FieldValuePair.java [113-115] public Object getValue() { - return values != null ? values : value; + return values != null ? java.util.Collections.unmodifiableList(values) : value; } Suggestion importance[1-10]: 4 __ Why: Returning an unmodifiable view provides defensive encapsulation, but the internal usage is controlled and the values are consumed by trusted writer code, limiting the practical risk.	Low
	Ensure child vector capacity before writes List.of(parseValue) throws NullPointerException on a null element, but null was already handled above; however, if parseValue is a List that itself contains nulls, the interior nulls are correctly handled. More importantly, ensure dataVector has sufficient capacity before writing at start + i — for large multi-valued documents the child vector may need reallocation. Consider calling dataVector.setInitialCapacity or using a setSafe-style write to avoid buffer overflows when the child vector's capacity was sized for scalar rows. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/fields/ParquetField.java [131-149] private void writeList(MappedFieldType fieldType, ManagedVSR managedVSR, ListVector listVector, Object parseValue) { int row = managedVSR.getRowCount(); if (parseValue == null) { listVector.setNull(row); return; } List<?> values = parseValue instanceof List<?> list ? list : List.of(parseValue); int start = listVector.startNewValue(row); FieldVector dataVector = listVector.getDataVector(); + int required = start + values.size(); + while (dataVector.getValueCapacity() < required) { + dataVector.reAlloc(); + } for (int i = 0; i < values.size(); i++) { Object value = values.get(i); if (value == null) { dataVector.setNull(start + i); } else { addToVector(dataVector, start + i, value); } } listVector.endValue(row, values.size()); } Suggestion importance[1-10]: 3 __ Why: The concrete addToVector implementations use setSafe which handles reallocation automatically (as seen in KeywordParquetField). The suggestion's added capacity management is likely redundant and may not address a real bug.	Low

[github-actions]

✅ Gradle check result for 384dc85: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.39%. Comparing base (bacf3f6) to head (5559906).
⚠️ Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
...a/org/opensearch/index/mapper/FilterFieldType.java	0.00%	3 Missing ⚠️
...a/org/opensearch/index/mapper/MappedFieldType.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22685      +/-   ##
============================================
- Coverage     71.39%   71.39%   -0.01%     
+ Complexity    76808    76781      -27     
============================================
  Files          6148     6148              
  Lines        357994   358012      +18     
  Branches      52179    52179              
============================================
+ Hits         255607   255610       +3     
+ Misses        82054    82022      -32     
- Partials      20333    20380      +47     

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

PR Reviewer Guide 🔍

(Review updated until commit 5559906)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

📝 TODO sections sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/fields/ArrowSchemaBuilder.java [48]: Get the mapping version while creating the schema sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/ArrowCalciteTypes.java [54]: TIMESTAMP_WITH_LOCAL_TIME_ZONE, DATE, TIME, DECIMAL still missing.

🔀 Multiple PR themes Sub-PR theme: Fix ColumnPath for LIST columns in writer properties Relevant files: sandbox/plugins/parquet-data-format/src/main/rust/src/writer_properties_builder.rs Sub-PR theme: Resolve nested column leaves in page-index schema resolver Relevant files: sandbox/plugins/analytics-backend-datafusion/rust/src/cache/page_index/column_schema_resolver.rs sandbox/plugins/analytics-backend-datafusion/rust/src/cache/page_index/page_index_io.rs Sub-PR theme: Core multi_value write path and mapping parameter Relevant files: sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/fields/ParquetField.java sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/fields/ArrowSchemaBuilder.java sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/writer/ParquetDocumentInput.java sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/writer/FieldValuePair.java sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/vsr/VSRManager.java server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java server/src/main/java/org/opensearch/index/mapper/MappedFieldType.java

⚡ Recommended focus areas for review Mutability contract violation The class Javadoc previously advertised immutability, and callers may still assume it. multiValued() now returns a mutable FieldValuePair whose internal values list is exposed directly via getValue(). A downstream consumer that retains and mutates the returned list, or one that reads it concurrently with a subsequent addValue, will observe inconsistent state. Consider returning an unmodifiable view or documenting the shared-mutable contract explicitly at getValue(). if (values == null) { throw new IllegalStateException("Cannot add a value to a single-valued FieldValuePair for [" + fieldType.name() + "]"); } values.add(nextValue); } /** Returns whether this pair accumulates multiple values into a list column. */ public boolean isMultiValued() { return values != null; } /** Returns the number of values held: always 1 for a scalar pair. */ public int valueCount() { return values == null ? 1 : values.size(); } /** * Returns the field type. * * @return the mapped field type */ public MappedFieldType getFieldType() { return fieldType; } /** * Returns the value: the single parsed value, or the {@code List} of values for a * multi-valued pair. * * @return the parsed field value(s) */ public Object getValue() { return values != null ? values : value; } List row write assumes current row writeList uses managedVSR.getRowCount() as the target row and calls listVector.startNewValue(row)/endValue. If createField is ever invoked for a row other than the current append position (e.g. out-of-order writes, or after the row count has advanced), the list offsets will be written at the wrong index and silently corrupt neighbouring rows. The scalar path derives its position the same way, so the assumption is at least consistent, but this should be asserted or documented at the API boundary. private void writeList(MappedFieldType fieldType, ManagedVSR managedVSR, ListVector listVector, Object parseValue) { int row = managedVSR.getRowCount(); if (parseValue == null) { listVector.setNull(row); return; } List<?> values = parseValue instanceof List<?> list ? list : List.of(parseValue); int start = listVector.startNewValue(row); FieldVector dataVector = listVector.getDataVector(); for (int i = 0; i < values.size(); i++) { Object value = values.get(i); if (value == null) { dataVector.setNull(start + i); } else { addToVector(dataVector, start + i, value); } } listVector.endValue(row, values.size()); } IdentityHashMap key stability seen is an IdentityHashMap<MappedFieldType, FieldValuePair>, so deduplication depends on the same MappedFieldType instance being passed for every addField call within one document. If the mapper ever hands back distinct instances for the same field name (e.g. a wrapper/filter type resolved twice), each call would create a new pair and the multi-value accumulation would silently split into multiple entries. Worth verifying the invariant holds across all call sites, or keying by field name. private final Map<MappedFieldType, FieldValuePair> seen = new IdentityHashMap<>(); private long rowId = -1; private boolean isClosed = false; @Override public void addField(MappedFieldType fieldType, Object value) { ensureOpen(); Set<FieldTypeCapabilities.Capability> capabilities = fieldType.getCapabilityMap() .getOrDefault(ParquetDataFormatPlugin.PARQUET_DATA_FORMAT, Set.of()); if (capabilities.isEmpty() && fieldType != PrimaryTermFieldType.INSTANCE) { // nothing to support on this format for this field. logger.trace("Ignored to add field: {} {}", fieldType.name(), fieldType.getCapabilityMap()); return; } FieldValuePair existing = seen.get(fieldType); if (existing == null) { // Fields declared `multi_value: true` in the mapping start out as a list of one so the // value shape reaching the VSR is the same whether the document had one value or several. FieldValuePair pair = fieldType.isMultiValued() ? FieldValuePair.multiValued(fieldType, value) : new FieldValuePair(fieldType, value); seen.put(fieldType, pair); collectedFields.add(pair); return; } if (existing.isMultiValued() == false) { throw new MapperParsingException( "Cannot accept multiple values for field: [" + fieldType.name() + "] of type: [" + fieldType.typeName() + "]. Set [multi_value: true] on the field mapping to store multiple values." ); } existing.addValue(value);

[github-actions]

Persistent review updated to latest commit 4041296

[github-actions]

❌ Gradle check result for 4041296: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 929e6d4

[github-actions]

✅ Gradle check result for 929e6d4: SUCCESS

[github-actions]

Persistent review updated to latest commit c53d71f

[github-actions]

✅ Gradle check result for c53d71f: SUCCESS

[github-actions]

Persistent review updated to latest commit cdd8f96

[github-actions]

✅ Gradle check result for cdd8f96: SUCCESS

[github-actions]

Persistent review updated to latest commit 3353037

[github-actions]

Persistent review updated to latest commit 8cba589

[github-actions]

Persistent review updated to latest commit 827515b

[github-actions]

✅ Gradle check result for 827515b: SUCCESS

[github-actions]

Persistent review updated to latest commit 5559906

[github-actions]

✅ Gradle check result for 5559906: SUCCESS