Add low_cardinality_enable field-level property for keyword/text fields

[cocosz]

Add low_cardinality_enable field-level property for parquet composite indices

Description

Adds a new field-level index setting low_cardinality_enable for the parquet-data-format sandbox plugin. When set to true for a keyword or text field on a parquet composite index, it automatically applies two optimizations at index creation time:

1. Suppresses the Lucene inverted index (index=false) — avoids write amplification and reduces index size for fields that will only be queried via Parquet bloom filters rather than Lucene term queries.
2. Enables a Parquet bloom filter for that column — so point-lookup equality queries can skip entire row groups efficiently at the Parquet layer.

This is designed for low-cardinality string fields (e.g. status, region, event_type) in analytical parquet composite indices where exact-match filtering via bloom filters is the primary access pattern and the Lucene inverted index overhead is wasteful.

Usage

PUT /my-index
{
  "settings": {
    "index.pluggable.dataformat.enabled": true,
    "index.pluggable.dataformat": "composite",
    "index.composite.primary_data_format": "parquet",
    "index.composite.secondary_data_formats": ["lucene"],
    "index.parquet.low_cardinality_enable.field": ["status", "region"],
    "index.parquet.low_cardinality_enable.value": [true, true]
  },
  "mappings": {
    "properties": {
      "status": { "type": "keyword" },
      "region": { "type": "keyword" }
    }
  }
}

This is equivalent to setting "index": false on those fields in the mapping plus enabling index.parquet.bloom_filter_enabled for the same fields — but expressed as a single declarative intent.

---

Validation

All error paths enforced at index creation time via ParquetIndexCreationValidator:

Condition	Error
Field type is not keyword or text	low_cardinality_enable is only supported for keyword and text fields
Field name not present in mappings	does not exist in mappings
Setting used on a non-parquet index	does not use parquet data format
Duplicate field name in the array	Duplicate field '...'
Field/value arrays have different lengths	must have the same size
Invalid boolean value (e.g. "yes")	Must be true or false
value=false	Accepted, no-op (field stays indexed)

---

Testing

Manually verified on a live OpenSearch 3.8.0-SNAPSHOT node with the full sandbox plugin stack (composite-engine, parquet-data-format, analytics-backend-datafusion).

Lucene index suppression

The _field_caps API reports searchable=false for fields with low_cardinality_enable=true, confirming no inverted index is written. The result is identical to explicitly setting "index": false in the field mapping:

status (keyword): searchable=False   ← low_cardinality_enable=true
category (keyword): searchable=True  ← plain keyword, unaffected

Parquet bloom filter

Inspected the written Parquet file for bloom filter. A bloom filter is present only for the low_cardinality_enable=true field, and absent for unaffected fields — identical behavior to using index.parquet.bloom_filter_enabled.field/value explicitly:

column        bloom_filter_offset  bloom_filter_length
status                        630                   47   ← bloom written
category                     None                 None   ← no bloom (expected)

Correctness cases (11 total)

#	Case	Result
1	keyword fields with value=true	Accepted; searchable=false, bloom written
2	text field with value=true	Accepted; searchable=false, bloom written
3	value=false (explicit opt-out)	Accepted; field stays searchable, no bloom
4	low_cardinality_enable coexists with bloom_filter_enabled + encoding	All settings applied correctly; explicit entries take precedence
5	Settings round-trip via _settings API	field and value arrays persisted correctly
6	Non-text/keyword field type (double, integer, …)	Rejected at index creation
7	Field name not present in mappings	Rejected at index creation
8	Setting used on a non-parquet index	Rejected at index creation
9	Duplicate field name in the array	Rejected at index creation
10	Field/value arrays have different lengths	Rejected at index creation
11	Invalid boolean value (e.g. "yes")	Rejected at index creation

---

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit a8b8b7a.

Path	Line	Severity	Description
server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java	210	medium	Core server code reads sandbox plugin settings via hardcoded raw string keys ('index.parquet.low_cardinality_enable.field/value') to suppress Lucene indexing on fields. The same pattern is duplicated in TextFieldMapper.java. While gated behind PLUGGABLE_DATAFORMAT_ENABLED_SETTING and consistent with the stated feature, the tight coupling of core mapper behavior to plugin-specific string keys (outside any formal extension point) means any admin with index-settings write access can silently disable field indexing across keyword/text fields — making those fields unsearchable without an explicit mapping change. This is unusual architecture for a sandbox plugin and warrants a design review.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 2963b34)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Inconsistent validation semantics getLowCardinalityEnabledFields filters to only fields where the value is true, so validation in validateFieldConfigurations only runs for fields explicitly enabled. A user who sets low_cardinality_enable.value to false for a non-keyword/text field, or a nonexistent field, will not receive the field-type/existence validation error. Additionally, mismatched parallel-array lengths between .field and .value are handled silently by buildFieldMap (dependent on its implementation), which could mask user errors. Consider validating all declared field entries regardless of value, or explicitly validating array-length parity. public static Set<String> getLowCardinalityEnabledFields(Settings settings) { Map<String, Boolean> fieldMap = buildFieldMap( LOW_CARDINALITY_ENABLE_FIELD_SETTING.get(settings), LOW_CARDINALITY_ENABLE_VALUE_SETTING.get(settings), "low_cardinality_enable" ); Set<String> enabled = new java.util.HashSet<>(); for (Map.Entry<String, Boolean> entry : fieldMap.entrySet()) { if (Boolean.TRUE.equals(entry.getValue())) { enabled.add(entry.getKey()); } } return Collections.unmodifiableSet(enabled); } Fragile duplicated setting-key lookup The raw string keys index.parquet.low_cardinality_enable.field and .value are duplicated in both KeywordFieldMapper and TextFieldMapper, and again in ParquetSettings. If the setting name ever changes in the plugin, these server-side mappers will silently stop applying index=false, producing mismatched behavior (bloom filter set but Lucene index still built) without any error. Consider exposing these keys as public constants referenced via a stable string, or moving the mechanism behind a documented SPI to avoid silent drift. private boolean isLowCardinalityEnabled(BuilderContext context) { if (!context.indexSettings().getAsBoolean(IndexSettings.PLUGGABLE_DATAFORMAT_ENABLED_SETTING.getKey(), false)) { return false; } List<String> fields = context.indexSettings().getAsList("index.parquet.low_cardinality_enable.field", Collections.emptyList()); if (fields.isEmpty()) return false; List<String> values = context.indexSettings().getAsList("index.parquet.low_cardinality_enable.value", Collections.emptyList()); String fullName = buildFullName(context); for (int i = 0; i < fields.size(); i++) { if (fullName.equals(fields.get(i)) && i < values.size() && "true".equalsIgnoreCase(values.get(i))) { return true; } } return false; } Silent override of user setting build() unconditionally calls this.index.setValue(false) when isLowCardinalityEnabled returns true. If a user explicitly set "index": true in the mapping for a text/keyword field that is also listed in low_cardinality_enable, their explicit choice is silently overridden with no warning or error. Consider detecting an explicit conflicting user value and either failing fast at index creation or logging a clear warning. @Override public TextFieldMapper build(BuilderContext context) { if (isLowCardinalityEnabled(context)) { this.index.setValue(false); } Precedence may contradict user intent The comment states "Explicit fieldBloomFilterEnabled entries take precedence", and putIfAbsent implements that. However, if a user explicitly sets bloom_filter_enabled=false for a field while also enabling low_cardinality_enable=true for the same field, the result is that the Lucene index is suppressed (via the mapper change) but the bloom filter is also disabled — leaving the field effectively unqueryable. Consider validating this contradictory combination at index creation time. // Explicit fieldBloomFilterEnabled entries take precedence; low_cardinality fields get implicit bloom=true. Map<String, Boolean> effectiveFieldBfEnabled; Set<String> lcFields = nativeSettings.getLowCardinalityEnabledFields(); if (lcFields.isEmpty()) { effectiveFieldBfEnabled = nativeSettings.getFieldBloomFilterEnabled(); } else { effectiveFieldBfEnabled = new HashMap<>(nativeSettings.getFieldBloomFilterEnabled()); for (String field : lcFields) { effectiveFieldBfEnabled.putIfAbsent(field, Boolean.TRUE); } } var bfEnabled = toBoolMapArrays(call, effectiveFieldBfEnabled);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 2963b34

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard against null MapperService in validation The new validateFieldConfigurations overload dereferences mapperService via mapperService.fieldType(fieldName) inside the low-cardinality validation loop, but the deprecated overload passes null for mapperService. If any caller still uses the deprecated overload and provides a non-empty lowCardinalityEnabledFields, this will NPE. Add an explicit null check on mapperService before calling fieldType, or throw a clear error when it is null but low-cardinality fields are present. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetSettings.java [831-838] public static void validateFieldConfigurations( Map<String, String> fieldEncodings, Map<String, String> fieldCompressions, Map<String, Boolean> fieldBloomFilterEnabled, Set<String> lowCardinalityEnabledFields, Schema schema, MapperService mapperService ) { + if (!lowCardinalityEnabledFields.isEmpty() && mapperService == null) { + throw new IllegalStateException("MapperService is required to validate low_cardinality_enable fields"); + } Suggestion importance[1-10]: 6 __ Why: Valid defensive check: the deprecated overload passes null for mapperService, so if it is ever called with a non-empty lowCardinalityEnabledFields, an NPE would occur. Adding an explicit check improves error clarity, though currently the deprecated overload always passes Collections.emptySet().	Low
General	Detect misconfig even with false values The validator counts !lowCardinalityEnabledFields.isEmpty() as "hasParquetSettings", but this set only includes entries whose value is true. If a user configures low_cardinality_enable.value=false (or a mix), hasParquetSettings will not reflect those entries and validation of parallel-array integrity may be skipped. Consider using the raw parallel-array setting (or the full map) to determine hasParquetSettings so misconfigurations are still validated. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetSettings.java [693-706] +public static Set<String> getLowCardinalityEnabledFields(Settings settings) { + Map<String, Boolean> fieldMap = buildFieldMap( + LOW_CARDINALITY_ENABLE_FIELD_SETTING.get(settings), + LOW_CARDINALITY_ENABLE_VALUE_SETTING.get(settings), + "low_cardinality_enable" + ); + Set<String> enabled = new java.util.HashSet<>(); + for (Map.Entry<String, Boolean> entry : fieldMap.entrySet()) { + if (Boolean.TRUE.equals(entry.getValue())) { + enabled.add(entry.getKey()); + } + } + return Collections.unmodifiableSet(enabled); +} - Suggestion importance[1-10]: 4 __ Why: Valid observation that hasParquetSettings may miss configurations where all low_cardinality_enable.value are false, but the improved_code is identical to existing_code, providing no actual fix. lu lu </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Validate parallel-array size consistency</summary> ___ **The lookup silently ignores mismatched lengths between <code>fields</code> and <code>values</code> (via <code>i < </code><br><code>values.size()</code>). This can lead to the field being treated as low-cardinality only <br>when its position happens to fall in range, hiding user-configuration bugs. Consider <br>validating length equality up front and returning early or throwing when arrays <br>disagree, so users are not confused by silently ignored settings.** [server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java [219-226]](https://github.com/opensearch-project/OpenSearch/pull/22405/files#diff-fed732f1669406d882652c899cc07ec8b98f2e0f8b40b27a426927905537e2f0R219-R226) ```diff List<String> values = context.indexSettings().getAsList("index.parquet.low_cardinality_enable.value", Collections.emptyList()); +if (fields.size() != values.size()) { + return false; +} String fullName = buildFullName(context); for (int i = 0; i < fields.size(); i++) { - if (fullName.equals(fields.get(i)) && i < values.size() && "true".equalsIgnoreCase(values.get(i))) { + if (fullName.equals(fields.get(i)) && "true".equalsIgnoreCase(values.get(i))) { return true; } } Suggestion importance[1-10]: 3 __ Why: Minor robustness improvement; however, the parallel-array size validation is likely handled elsewhere (e.g., in ParquetSettings.buildFieldMap), so the mapper's raw lookup silently tolerating mismatches is acceptable as a fallback.	Low

---

Previous suggestions

Suggestions up to commit 4e27ed6

Category	Suggestion	Impact
Possible issue	Prevent NPE when mapperService is null mapperService can be null when this method is invoked via the deprecated overload, which will cause a NullPointerException if lowCardinalityEnabledFields is non-empty. Guard against a null mapperService before dereferencing it, or throw an explicit IllegalStateException explaining that mapper-based validation is required. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetSettings.java [882-888] +if (mapperService == null) { + throw new IllegalStateException( + "MapperService is required to validate low_cardinality_enable fields" + ); +} org.opensearch.index.mapper.MappedFieldType mappedFieldType = mapperService.fieldType(fieldName); if (mappedFieldType == null) { throw new IllegalArgumentException( "Field '" + fieldName + "' in low_cardinality_enable configuration does not exist in mappings" ); } String typeName = mappedFieldType.typeName(); Suggestion importance[1-10]: 6 __ Why: Valid concern: the deprecated overload passes null for mapperService, so if any caller uses it with non-empty lowCardinalityEnabledFields, an NPE would occur. The suggestion improves defensive handling, though in practice the deprecated overload always passes an empty set.	Low
General	Clarify precedence between conflicting settings The comment states "Explicit fieldBloomFilterEnabled entries take precedence" and putIfAbsent correctly preserves that — but if a user explicitly sets bloom_filter_enabled=false on a low_cardinality_enable=true field, bloom filters will be disabled, contradicting the documented behavior that low-cardinality fields "get implicit bloom=true". Clarify the intended precedence and either enforce bloom=true unconditionally for low-cardinality fields or reject the conflicting configuration at validation time. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/bridge/RustBridge.java [455-464] Map<String, Boolean> effectiveFieldBfEnabled; Set<String> lcFields = nativeSettings.getLowCardinalityEnabledFields(); if (lcFields.isEmpty()) { effectiveFieldBfEnabled = nativeSettings.getFieldBloomFilterEnabled(); } else { effectiveFieldBfEnabled = new HashMap<>(nativeSettings.getFieldBloomFilterEnabled()); for (String field : lcFields) { - effectiveFieldBfEnabled.putIfAbsent(field, Boolean.TRUE); + // low_cardinality_enable forces bloom filter on + effectiveFieldBfEnabled.put(field, Boolean.TRUE); } } Suggestion importance[1-10]: 5 __ Why: Legitimate concern about conflicting user intent between bloom_filter_enabled=false and low_cardinality_enable=true. The fix would enforce documented behavior, though the correct resolution (override vs. reject) is a design decision.	Low
	Guard against mismatched parallel arrays The lookup silently treats a missing/short values list as "not enabled", which can mask a misconfigured parallel-arrays setting where fields.size() != values.size(). Since ParquetSettings already enforces parallel-array alignment at validation, but this mapper runs during build, consider returning early or logging when sizes mismatch to avoid the mapper diverging from validator behavior. Also, this parallel scan will return the first match — if the same field name appears twice, later entries are ignored. server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java [219-225] List<String> values = context.indexSettings().getAsList("index.parquet.low_cardinality_enable.value", Collections.emptyList()); +if (fields.size() != values.size()) return false; String fullName = buildFullName(context); for (int i = 0; i < fields.size(); i++) { - if (fullName.equals(fields.get(i)) && i < values.size() && "true".equalsIgnoreCase(values.get(i))) { + if (fullName.equals(fields.get(i)) && "true".equalsIgnoreCase(values.get(i))) { return true; } } Suggestion importance[1-10]: 4 __ Why: Minor robustness improvement. The existing bounds check i < values.size() already prevents any out-of-bounds errors, and validation is enforced at index creation, so the practical impact is low.	Low
	Align semantics for false-valued entries getLowCardinalityEnabledFields silently drops entries whose value is false, but KeywordFieldMapper/TextFieldMapper and ParquetIndexCreationValidator treat the presence of a field name in the settings as sufficient to imply low-cardinality mode elsewhere. To keep behavior consistent, either also return the field names when the value is false (and let validation reject invalid types), or ensure downstream code checks the boolean value uniformly — otherwise a user setting low_cardinality_enable=false still triggers validation via hasParquetSettings but not the actual index=false behavior. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetSettings.java [699-705] +Set<String> enabled = new java.util.HashSet<>(); +for (Map.Entry<String, Boolean> entry : fieldMap.entrySet()) { + if (Boolean.TRUE.equals(entry.getValue())) { + enabled.add(entry.getKey()); + } +} +return Collections.unmodifiableSet(enabled); - Suggestion importance[1-10]: 3 __ Why: The existing_code and improved_code are identical, making the suggestion non-actionable. It raises a valid semantic concern but does not provide a concrete fix.	Low

Suggestions up to commit 1a94b08

Category	Suggestion	Impact
Possible issue	Gate low-cardinality on parquet primary format The check should also verify that the index is actually a Parquet composite index (i.e., index.composite.primary_data_format == "parquet"), matching the validation in ParquetIndexCreationValidator. Otherwise, if pluggable.dataformat.enabled=true is set without the parquet primary format, this will silently disable indexing on keyword fields based on settings that would never be enforced. server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java [214-226] +if (!context.indexSettings().getAsBoolean(IndexSettings.PLUGGABLE_DATAFORMAT_ENABLED_SETTING.getKey(), false) + || !"parquet".equals(context.indexSettings().get("index.composite.primary_data_format"))) { + return false; +} List<String> fields = context.indexSettings().getAsList("index.parquet.low_cardinality_enable.field", Collections.emptyList()); if (fields.isEmpty()) return false; List<String> values = context.indexSettings().getAsList("index.parquet.low_cardinality_enable.value", Collections.emptyList()); String fullName = buildFullName(context); for (int i = 0; i < fields.size(); i++) { if (fullName.equals(fields.get(i)) && i < values.size() && "true".equalsIgnoreCase(values.get(i))) { return true; } } return false; Suggestion importance[1-10]: 7 __ Why: Valid concern: the mapper-level check only gates on PLUGGABLE_DATAFORMAT_ENABLED_SETTING, but the validator also requires the primary data format to be parquet. Aligning this check prevents silently disabling indexing when parquet is not actually the primary format.	Medium
	Disable index-dependent text features when forcing index=false Forcing index=false on a text field will break index_phrases, index_prefixes, and analyzer-based features which typically require an indexed field; the subsequent buildPhraseMapper/prefix logic may throw or produce an inconsistent mapping. Add guards to disable/skip these dependent features (or reject the combination) when low-cardinality forces index=false on a text field. server/src/main/java/org/opensearch/index/mapper/TextFieldMapper.java [491-495] @Override public TextFieldMapper build(BuilderContext context) { if (isLowCardinalityEnabled(context)) { this.index.setValue(false); + // Dependent features require an indexed field; disable them to keep the mapping consistent. + this.indexPhrases.setValue(false); + this.indexPrefixes.setValue(null); } FieldType fieldType = TextParams.buildFieldType(index, store, indexOptions, norms, termVectors); Suggestion importance[1-10]: 6 __ Why: Legitimate concern that forcing index=false on a text field may conflict with index_phrases/index_prefixes, potentially causing errors or inconsistent mappings. The specific field names in improved_code may not exactly match, but the concern is valid.	Low
	Guard against null MapperService during validation mapperService may be null when invoked via the deprecated overload path, causing a NullPointerException if lowCardinalityEnabledFields is ever non-empty on that path. Explicitly guard against a null mapperService (or make the deprecated overload throw if the set is non-empty) to fail with a clear error. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetSettings.java [882-889] +if (mapperService == null) { + throw new IllegalStateException( + "MapperService is required to validate low_cardinality_enable fields" + ); +} org.opensearch.index.mapper.MappedFieldType mappedFieldType = mapperService.fieldType(fieldName); if (mappedFieldType == null) { throw new IllegalArgumentException( "Field '" + fieldName + "' in low_cardinality_enable configuration does not exist in mappings" ); } String typeName = mappedFieldType.typeName(); if (!"keyword".equals(typeName) && !"text".equals(typeName)) { Suggestion importance[1-10]: 5 __ Why: The deprecated overload passes null for mapperService with an empty set, so the NPE won't occur in current usage. However, adding a defensive null-check improves robustness for future callers.	Low
General	Handle false entries consistently for validation Silently dropping entries where the value is false makes the parallel-array semantics inconsistent with other field settings (e.g., bloom_filter_enabled) and also hides invalid mappings from validation (a false entry pointing at a non-keyword/text field will never be caught). Consider either validating all entries (including false ones) or documenting/rejecting false values explicitly to avoid surprising behavior. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetSettings.java [699-705] Set<String> enabled = new java.util.HashSet<>(); for (Map.Entry<String, Boolean> entry : fieldMap.entrySet()) { if (Boolean.TRUE.equals(entry.getValue())) { enabled.add(entry.getKey()); } } +// Note: field-name/type validation is applied only to enabled entries. return Collections.unmodifiableSet(enabled); Suggestion importance[1-10]: 4 __ Why: Valid observation about inconsistent handling of false entries which could hide invalid mappings. However, the improved_code only adds a comment without a functional change, limiting its impact.	Low

Suggestions up to commit 1e1763e

Category	Suggestion	Impact
Possible issue	Force bloom filter on for low-cardinality fields Using putIfAbsent means an explicit fieldBloomFilterEnabled=false will suppress the implicit bloom filter for a low-cardinality field, which contradicts the documented behavior ("low_cardinality fields get implicit bloom=true"). If low-cardinality bloom filter is required, use put to force true, or explicitly reject conflicting configurations at validation time. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/bridge/RustBridge.java [455-464] Map<String, Boolean> effectiveFieldBfEnabled; Set<String> lcFields = nativeSettings.getLowCardinalityEnabledFields(); if (lcFields.isEmpty()) { effectiveFieldBfEnabled = nativeSettings.getFieldBloomFilterEnabled(); } else { effectiveFieldBfEnabled = new HashMap<>(nativeSettings.getFieldBloomFilterEnabled()); for (String field : lcFields) { - effectiveFieldBfEnabled.putIfAbsent(field, Boolean.TRUE); + effectiveFieldBfEnabled.put(field, Boolean.TRUE); } } Suggestion importance[1-10]: 7 __ Why: The comment states "Explicit fieldBloomFilterEnabled entries take precedence", so putIfAbsent is intentional per the documented behavior. However, the suggestion validly points out potential inconsistency with the "implicit bloom=true" documentation, warranting clarification or validation.	Medium
General	Detect conflict with explicit index option Force-setting indexed to false in build() silently overrides any explicit index: true from the user mapping without warning. Consider throwing an error when both are set to conflicting values, or at minimum logging a warning, so users are not surprised when their explicit mapping option is ignored. server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java [217-228] List<String> fields = context.indexSettings() .getAsList("index.parquet.low_cardinality_enable.field", Collections.emptyList()); if (fields.isEmpty()) return false; List<String> values = context.indexSettings() .getAsList("index.parquet.low_cardinality_enable.value", Collections.emptyList()); String fullName = buildFullName(context); for (int i = 0; i < fields.size(); i++) { if (fullName.equals(fields.get(i)) && i < values.size() && "true".equalsIgnoreCase(values.get(i))) { + if (this.indexed.isConfigured() && Boolean.TRUE.equals(this.indexed.getValue())) { + throw new IllegalArgumentException( + "Field '" + fullName + "' has index=true but low_cardinality_enable requires index=false" + ); + } return true; } } return false; Suggestion importance[1-10]: 6 __ Why: Silently overriding user-configured index: true in the mapping could be surprising and confusing. Detecting the conflict and failing/logging improves user experience, though it's a UX concern, not a bug.	Low
	Validate missing values in parallel arrays buildFieldMap likely throws when the field and value lists have different sizes, but if it tolerates empty value lists you may silently drop entries. More importantly, entries with false values are silently ignored here — consider validating that all listed fields have a corresponding value entry and logging/throwing on unexpected null values to catch misconfiguration early. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetSettings.java [699-705] Set<String> enabled = new java.util.HashSet<>(); for (Map.Entry<String, Boolean> entry : fieldMap.entrySet()) { + if (entry.getValue() == null) { + throw new IllegalArgumentException( + "Missing low_cardinality_enable value for field '" + entry.getKey() + "'" + ); + } if (Boolean.TRUE.equals(entry.getValue())) { enabled.add(entry.getKey()); } } return Collections.unmodifiableSet(enabled); Suggestion importance[1-10]: 3 __ Why: The parallel-array size mismatch is likely already validated by buildFieldMap, and null values are unlikely in this flow. The suggestion adds marginal defensive validation.	Low

Suggestions up to commit dbab29d

Category	Suggestion	Impact
Possible issue	Guard against null MapperService before dereference The validate overload can be called with mapperService == null (deprecated overload passes null). Additionally, when invoked via ParquetIndexCreationValidator with a non-empty lowCardinalityEnabledFields, calling mapperService.fieldType(...) on a null reference will throw an NPE instead of a clear validation error. Add an explicit null check before dereferencing. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetSettings.java [882-888] +if (mapperService == null) { + throw new IllegalStateException( + "MapperService is required to validate low_cardinality_enable fields" + ); +} org.opensearch.index.mapper.MappedFieldType mappedFieldType = mapperService.fieldType(fieldName); if (mappedFieldType == null) { throw new IllegalArgumentException( "Field '" + fieldName + "' in low_cardinality_enable configuration does not exist in mappings" ); } String typeName = mappedFieldType.typeName(); Suggestion importance[1-10]: 6 __ Why: Valid defensive check: the deprecated overload passes null for mapperService, and if any caller uses that overload with non-empty low-cardinality fields (unlikely since the deprecated overload passes emptySet), an NPE could occur. The concern is somewhat theoretical but reasonable.	Low
General	Fail fast on mismatched parallel arrays getLowCardinalityEnabledFields silently drops fields whose value is false, so a user mistakenly setting the field with a false value gets no signal. Also, since KeywordFieldMapper/TextFieldMapper bypass this helper and read raw settings, ensure both paths interpret the parallel arrays consistently (both include only true values) — currently the mapper checks "true".equalsIgnoreCase(...) which matches, but any parsing divergence (e.g., invalid boolean) will crash validateBoolean but not the mapper. Consider validating list-size parity here explicitly to fail fast. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetSettings.java [693-706] public static Set<String> getLowCardinalityEnabledFields(Settings settings) { - Map<String, Boolean> fieldMap = buildFieldMap( - LOW_CARDINALITY_ENABLE_FIELD_SETTING.get(settings), - LOW_CARDINALITY_ENABLE_VALUE_SETTING.get(settings), - "low_cardinality_enable" - ); + List<String> fields = LOW_CARDINALITY_ENABLE_FIELD_SETTING.get(settings); + List<Boolean> values = LOW_CARDINALITY_ENABLE_VALUE_SETTING.get(settings); + if (fields.size() != values.size()) { + throw new IllegalArgumentException( + "low_cardinality_enable field/value lists must have the same length" + ); + } + Map<String, Boolean> fieldMap = buildFieldMap(fields, values, "low_cardinality_enable"); Set<String> enabled = new java.util.HashSet<>(); for (Map.Entry<String, Boolean> entry : fieldMap.entrySet()) { if (Boolean.TRUE.equals(entry.getValue())) { enabled.add(entry.getKey()); } } return Collections.unmodifiableSet(enabled); } Suggestion importance[1-10]: 4 __ Why: Adding explicit list-size parity validation could improve error clarity, but buildFieldMap likely already handles this. Minor improvement in error handling.	Low
	Bound iteration by min of both list sizes The lookup uses buildFullName(context), but buildFullName may not be safe/idempotent to call twice during build (once here and again inside buildFieldType). Cache the result in a local variable used both here and in the subsequent build, or ensure buildFullName has no side effects to prevent divergent behavior across calls. server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java [217-228] List<String> fields = context.indexSettings() .getAsList("index.parquet.low_cardinality_enable.field", Collections.emptyList()); if (fields.isEmpty()) return false; List<String> values = context.indexSettings() .getAsList("index.parquet.low_cardinality_enable.value", Collections.emptyList()); -String fullName = buildFullName(context); -for (int i = 0; i < fields.size(); i++) { - if (fullName.equals(fields.get(i)) && i < values.size() && "true".equalsIgnoreCase(values.get(i))) { +final String fullName = buildFullName(context); +final int n = Math.min(fields.size(), values.size()); +for (int i = 0; i < n; i++) { + if (fullName.equals(fields.get(i)) && "true".equalsIgnoreCase(values.get(i))) { return true; } } Suggestion importance[1-10]: 3 __ Why: The existing code already includes an i < values.size() bound check, so the suggested change is essentially equivalent. The buildFullName concern is speculative and not backed by evidence.	Low

Suggestions up to commit aaa60b0

Category	Suggestion	Impact
Possible issue	Guard against null MapperService NPE The deprecated overload passes mapperService=null, but the new validation path dereferences mapperService.fieldType(...) whenever lowCardinalityEnabledFields is non-empty. If any caller ever invokes the deprecated overload with low-cardinality fields, this will NPE; guard against a null mapperService explicitly. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetSettings.java [882-887] +if (mapperService == null) { + throw new IllegalStateException( + "MapperService is required to validate low_cardinality_enable for field '" + fieldName + "'" + ); +} org.opensearch.index.mapper.MappedFieldType mappedFieldType = mapperService.fieldType(fieldName); if (mappedFieldType == null) { throw new IllegalArgumentException( "Field '" + fieldName + "' in low_cardinality_enable configuration does not exist in mappings" ); } Suggestion importance[1-10]: 5 __ Why: The deprecated overload passes an empty set for low-cardinality fields so the loop won't execute, making the NPE unreachable in practice. Still, an explicit null guard is a reasonable defensive measure for future callers.	Low
	Avoid parallel-array size mismatch failure buildFieldMap likely requires the field and value lists to have equal size, so calling it with an empty value list (when only the field key is present) may fail before filtering by true. Build the map directly from the parallel field/value lists here to gracefully handle mismatched or empty inputs and to correctly collect only fields whose value is true. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/ParquetSettings.java [699-705] +List<String> fields = LOW_CARDINALITY_ENABLE_FIELD_SETTING.get(settings); +List<Boolean> values = LOW_CARDINALITY_ENABLE_VALUE_SETTING.get(settings); Set<String> enabled = new java.util.HashSet<>(); -for (Map.Entry<String, Boolean> entry : fieldMap.entrySet()) { - if (Boolean.TRUE.equals(entry.getValue())) { - enabled.add(entry.getKey()); +int n = Math.min(fields.size(), values.size()); +for (int i = 0; i < n; i++) { + if (Boolean.TRUE.equals(values.get(i))) { + enabled.add(fields.get(i)); } } return Collections.unmodifiableSet(enabled); Suggestion importance[1-10]: 4 __ Why: The suggestion is speculative about buildFieldMap's behavior without evidence it fails on empty/mismatched lists; the existing implementation is likely already handled by buildFieldMap. Minor defensive improvement at best.	Low
General	Detect conflicting explicit index setting Silently overriding a user-specified index: true mapping to false can mask configuration errors. Only apply the override when indexed was not explicitly set by the user, or throw a clear error if both are explicitly set to conflicting values, to preserve mapping semantics. server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java [213-226] List<String> fields = context.indexSettings() .getAsList("index.parquet.low_cardinality_enable.field", Collections.emptyList()); if (fields.isEmpty()) return false; List<String> values = context.indexSettings() .getAsList("index.parquet.low_cardinality_enable.value", Collections.emptyList()); String fullName = buildFullName(context); for (int i = 0; i < fields.size(); i++) { if (fullName.equals(fields.get(i)) && i < values.size() && "true".equalsIgnoreCase(values.get(i))) { + if (this.indexed.isConfigured() && Boolean.TRUE.equals(this.indexed.getValue())) { + throw new IllegalArgumentException( + "Field [" + fullName + "] cannot set index=true when low_cardinality_enable is true" + ); + } return true; } } return false; Suggestion importance[1-10]: 5 __ Why: Raising an error on conflicting explicit index=true with low_cardinality_enable is a reasonable UX improvement to prevent silent overrides, though it's a design choice rather than a bug fix.	Low

[github-actions]

❌ Gradle check result for aaa60b0: 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 dbab29d

[github-actions]

Persistent review updated to latest commit 1e1763e

[github-actions]

❌ Gradle check result for 1e1763e: 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 1a94b08

[github-actions]

Persistent review updated to latest commit 4e27ed6

[github-actions]

Persistent review updated to latest commit 2963b34

[github-actions]

✅ Gradle check result for 2963b34: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 25.00000% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.37%. Comparing base (0a75b13) to head (2963b34).

Files with missing lines	Patch %	Lines
...rg/opensearch/index/mapper/KeywordFieldMapper.java	25.00%	7 Missing and 2 partials ⚠️
...a/org/opensearch/index/mapper/TextFieldMapper.java	25.00%	7 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22405      +/-   ##
============================================
- Coverage     73.48%   73.37%   -0.11%     
+ Complexity    76464    76326     -138     
============================================
  Files          6101     6101              
  Lines        346488   346512      +24     
  Branches      49871    49881      +10     
============================================
- Hits         254620   254263     -357     
- Misses        71599    71959     +360     
- Partials      20269    20290      +21     

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

[Bukhtawar]

The one thing I need to understand is what if the cardinality is very low like HTTP status codes, will low_cardinality be any significant value? It definitely is for high_cardinality but at the expense of low performance. How do we highlight this positiong?