[Pluggable Dataformat] Adding support for FieldTypeCapabilities

[darjisagar7]

Description

[Describe what this change achieves]

Related Issues

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

Check List

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

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 3295038)

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 In testIOExceptionDoesNotAffectBufferedDeletes, the test verifies that buffered deletes recorded before and after an IOException remain intact. However, the test does not verify that the deleteDoc call that threw the exception did not partially modify internal state. If the writer's deleteDocument method modifies state before throwing, the deleter's internal consistency could be compromised. The test should verify that the deleter remains fully operational after the exception, not just that buffered deletes are preserved. IOException expectedException = new IOException("disk error"); when(mockWriter.deleteDocument(any(DeleteInput.class))).thenThrow(expectedException); deleter.recordBufferedDeletes("doc1"); try { deleter.deleteDoc(new DeleteInput("_id", "doc2", 1L)); fail("Expected IOException"); } catch (IOException e) { // expected } deleter.recordBufferedDeletes("doc3"); assertTrue(deleter.isActive()); Queue<String> buffered = deleter.deactivate(); assertEquals(2, buffered.size()); assertTrue(buffered.contains("doc1")); assertTrue(buffered.contains("doc3")); } Possible Issue In testKeywordIgnoreAboveIndexAndVerify, the test assumes that when a keyword value exceeds ignore_above, the field value in parquet will be null and the raw value will be stored in _ignored_source.field. However, if the sourceKeywordFieldType storage logic changes or is not yet implemented, this assertion could fail silently or produce false positives. The test should verify that the _ignored_source.field key exists before asserting its value to avoid NullPointerException if the feature is incomplete. assertNull("field should be null for doc2 (exceeds ignore_above)", doc2Row.get("field")); assertEquals( "sourceKeywordFieldType should store raw value for doc2", "this_exceeds_ignore_above_limit", doc2Row.get("_ignored_source.field") ); Possible Issue In verifyParquetSortOrder, the test iterates over parquet rows and asserts sort order constraints. If a parquet file contains zero or one row, the loop body (lines 144-179) is skipped via 'continue' at line 142. However, if all segments have <=1 row, the test passes without verifying any sort order. This could mask a bug where sort order is never applied. The test should fail or log a warning if no multi-row segments are found, ensuring that sort order verification actually occurred. Path parquetDir = getParquetDir(); for (Segment segment : snapshot.getSegments()) { WriterFileSet wfs = segment.dfGroupedSearchableFiles().get("parquet"); for (String file : wfs.files()) { Path filePath = parquetDir.resolve(file); String json = RustBridge.readAsJson(filePath.toString()); List<Map<String, Object>> rows; try ( XContentParser parser = JsonXContent.jsonXContent.createParser( NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, json ) ) { rows = parser.list().stream().map(o -> { @SuppressWarnings("unchecked") Map<String, Object> m = (Map<String, Object>) o; return m; }).toList(); } if (rows.size() <= 1) continue; for (int i = 1; i < rows.size(); i++) { Object prevAge = rows.get(i - 1).get("age"); Object currAge = rows.get(i).get("age"); // nulls first for age if (prevAge == null && currAge == null) continue; if (prevAge == null) continue; if (currAge == null) { fail("age null should come before non-null at row " + i); } int prevAgeVal = ((Number) prevAge).intValue(); int currAgeVal = ((Number) currAge).intValue(); assertTrue( "age should be DESC but found " + prevAgeVal + " before " + currAgeVal + " at row " + i + " in " + file, prevAgeVal >= currAgeVal ); // When age is equal, verify name ASC (nulls last) if (prevAgeVal == currAgeVal) { Object prevName = rows.get(i - 1).get("name"); Object currName = rows.get(i).get("name"); if (prevName != null && currName == null) continue; if (prevName == null && currName != null) { fail("name nulls should be last at row " + i + " in " + file); } if (prevName != null && currName != null) { assertTrue( "name should be ASC but found '" + prevName + "' before '" + currName + "' at row " + i + " in " + file, ((String) prevName).compareTo((String) currName) <= 0 ); } } } } } } Possible Issue In testMappingUpdateFailsWithUnsupportedField, the test expects the cluster to detect a shard failure after an unsupported mapping update. The test uses assertBusy to wait for the index health to become RED or YELLOW. However, if the cluster does not propagate the failure quickly or if the shard remains GREEN due to a bug in failure detection, the assertBusy could time out and fail the test. The test should also verify that the specific shard has failed (e.g., by checking shard allocation status) to ensure the failure is detected as expected, not just that the cluster health changed for an unrelated reason. assertBusy(() -> { String health = client().admin().cluster().prepareHealth(indexName).get().getStatus().name(); assertTrue( "Index should be RED or YELLOW after unsupported mapping update, got: " + health, health.equals("RED") || health.equals("YELLOW") ); });

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 3295038

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Ensure executor shutdown after futures The test uses Future.get() with a timeout to wait for threads to complete. If a thread hangs or takes longer than expected, the test will throw a TimeoutException, but the executor is not shut down, potentially leaving threads running. Always shut down the executor in a finally block or after all futures complete to prevent thread leaks. sandbox/plugins/analytics-backend-lucene/src/test/java/org/opensearch/be/lucene/index/DeleterImplTests.java [372-374] -public void testConcurrentDeleteAndDeactivate() throws Exception { - ... +try { deleteFuture.get(10, TimeUnit.SECONDS); bufferFuture.get(10, TimeUnit.SECONDS); deactivateFuture.get(10, TimeUnit.SECONDS); +} finally { + executor.shutdown(); +} Suggestion importance[1-10]: 8 __ Why: The suggestion identifies a potential thread leak if any Future.get() call throws a TimeoutException. The executor is shut down at line 380, but only if all futures complete successfully. Wrapping the get() calls in a try-finally block ensures the executor is always shut down, preventing resource leaks in test failures.	Medium
	Verify key existence before assertion The assertion logic assumes that when ignore_above is exceeded, the main field is null and the raw value is stored in _ignored_source.field. However, the test does not verify that the _ignored_source.field key actually exists before calling get(). If the key is missing, get() returns null, and the assertion will fail with a misleading message. Add an explicit check that the key exists before asserting its value. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/CompositeFieldCapabilityIT.java [498-503] -public void testKeywordIgnoreAboveIndexAndVerify() throws Exception { - ... - // doc2: value exceeds ignore_above — field is null (ignored), source keyword stores raw value - assertNull("field should be null for doc2 (exceeds ignore_above)", doc2Row.get("field")); - assertEquals( - "sourceKeywordFieldType should store raw value for doc2", - "this_exceeds_ignore_above_limit", - doc2Row.get("_ignored_source.field") - ); +assertNull("field should be null for doc2 (exceeds ignore_above)", doc2Row.get("field")); +assertTrue("_ignored_source.field should exist for doc2", doc2Row.containsKey("_ignored_source.field")); +assertEquals( + "sourceKeywordFieldType should store raw value for doc2", + "this_exceeds_ignore_above_limit", + doc2Row.get("_ignored_source.field") +); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the test assumes _ignored_source.field exists without verifying it first. Adding an explicit containsKey() check before the assertion improves test robustness and provides clearer failure messages if the key is missing.	Medium
	Update error message for clarity The error message only mentions INDEX_DERIVED_SOURCE_SETTING but the condition now also checks PLUGGABLE_DATAFORMAT_ENABLED_SETTING. Update the error message to reflect both settings that prevent disabling _source. server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java [187-192] if ((context.indexSettings().getAsBoolean(IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey(), false) || context.indexSettings().getAsBoolean(IndexSettings.PLUGGABLE_DATAFORMAT_ENABLED_SETTING.getKey(), false)) && !enabled.getValue()) { throw new MapperParsingException( - "_source can't be disabled with " + IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey() + " enabled index setting" + "_source can't be disabled when " + IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey() + " or " + IndexSettings.PLUGGABLE_DATAFORMAT_ENABLED_SETTING.getKey() + " is enabled" ); Suggestion importance[1-10]: 7 __ Why: The error message should accurately reflect both conditions that prevent disabling _source. The current message only mentions INDEX_DERIVED_SOURCE_SETTING but the condition now checks both INDEX_DERIVED_SOURCE_SETTING and PLUGGABLE_DATAFORMAT_ENABLED_SETTING, making the error message misleading.	Medium
	Add trace logging for skipped fields The early return when capabilities are empty silently skips fields without logging. For debugging capability assignment issues, consider adding trace-level logging when fields are skipped due to empty capabilities. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/index/LuceneDocumentInput.java [90-94] Set<FieldTypeCapabilities.Capability> capabilities = fieldType.getCapabilityMap().getOrDefault(LucenePlugin.DATA_FORMAT, Set.of()); if (capabilities.isEmpty()) { - // nothing to support on this format for this field. + logger.trace("Skipping field [{}] of type [{}]: no capabilities assigned for Lucene format", fieldType.name(), fieldType.typeName()); return; } Suggestion importance[1-10]: 6 __ Why: Good suggestion to add trace logging for debugging capability assignment issues. This would help developers understand why fields are being skipped without adding overhead in production (trace level is typically disabled).	Low
	Verify method accessibility The validation for pluggable data format occurs after checking the type but before setting nested = true. If the exception is thrown, the nested flag won't be set, which is correct. However, ensure isPluggableDataFormatEnabled method exists and is accessible in this context. server/src/main/java/org/opensearch/index/mapper/ObjectMapper.java [422-424] +if (type.equals(NESTED_CONTENT_TYPE)) { + if (isPluggableDataFormatEnabled(parserContext.getSettings())) { + throw new MapperParsingException("nested type is not supported with pluggable data format on field [" + name + "]"); + } + nested = true; - Suggestion importance[1-10]: 6 __ Why: The suggestion asks to verify that isPluggableDataFormatEnabled method exists and is accessible. While this is a valid concern for code correctness, the suggestion doesn't identify an actual problem with the code - it's asking for verification rather than pointing out a definite issue.	Low
	Validate constructed field name The method constructs a field name by concatenating "_ignored_source." with fieldType().name(). If fieldType().name() contains special characters (e.g., dots, which are valid in field names), the resulting field name may conflict with existing fields or violate naming conventions. Validate or sanitize the field name to prevent potential conflicts or unexpected behavior. server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java [887-892] private KeywordFieldType buildRawKeywordValueFieldType() { if (isIneligibleForGeneratingSource() && canConsumeRawValueForSource) { - return new KeywordFieldType("_ignored_source." + fieldType().name(), false, true, false, false, fieldType().meta()); + String rawFieldName = "_ignored_source." + fieldType().name(); + // Validate or sanitize rawFieldName if necessary + return new KeywordFieldType(rawFieldName, false, true, false, false, fieldType().meta()); } return null; } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about potential field name conflicts when concatenating "_ignored_source." with fieldType().name(). However, the improved code only adds a comment without actual validation logic. The concern is legitimate but the proposed fix is incomplete, making this a moderate-value suggestion.	Low
	Consolidate capability assignment logic The capability assignment logic is duplicated in the constructor and in assignCapabilitiesRecursive. This creates a maintenance burden and potential inconsistency. Consider consolidating the logic into a single method that handles both regular and metadata fields. server/src/main/java/org/opensearch/index/mapper/DocumentMapper.java [229-236] final DataFormatRegistry registry = mapperService.documentMapperParser().getDataFormatRegistry(); if (indexSettings.isPluggableDataFormatEnabled() && registry != null) { + assignAllCapabilities(mapping, registry, indexSettings); +} + +private void assignAllCapabilities(Mapping mapping, DataFormatRegistry registry, IndexSettings indexSettings) { assignCapabilitiesRecursive(mapping.root(), registry, indexSettings); for (MetadataFieldMapper metadataMapper : mapping.metadataMappers) { registry.assignCapabilities(metadataMapper.fieldType(), indexSettings); } } Suggestion importance[1-10]: 5 __ Why: Valid refactoring suggestion to reduce duplication between constructor and assignCapabilitiesRecursive. However, the impact is moderate since the duplicated code is minimal (just a loop over metadata mappers) and the current structure is clear.	Low
	Optimize error message formatting The error message construction uses Collectors.toList() which creates an unnecessary intermediate collection. For error messages, using Collectors.joining(", ") would be more efficient and produce cleaner output. sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeDataFormatPlugin.java [396-409] if (remaining.isEmpty() == false) { throw new MapperParsingException( "Field [" + fieldType.name() + "] of type [" + typeName + "] requires capabilities " + requested + " but configured data formats cannot collectively cover: " + remaining + ". Configured formats: " - + formats.stream().map(DataFormat::name).collect(Collectors.toList()) + + formats.stream().map(DataFormat::name).collect(Collectors.joining(", ")) ); } Suggestion importance[1-10]: 4 __ Why: Minor optimization suggestion using Collectors.joining(", ") instead of Collectors.toList() for error messages. The improvement is marginal since this is an error path, but the suggested code is cleaner and more idiomatic.	Low
	Remove redundant null validation The null check throws an exception after verifying capabilities, but the assertion at the method start already enforces non-null values. This creates redundant validation. Consider removing the assertion or the explicit null check to avoid confusion about which validation is authoritative. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/index/LuceneDocumentInput.java [95-99] -if (value == null) { - throw new IllegalArgumentException( - "Field value must not be null for: " + fieldType.name() + " of type: " + fieldType.typeName() - ); -} +assert value != null : "Field value must not be null for: " + fieldType.name() + " of type: " + fieldType.typeName(); Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies redundant validation logic, but the assertion was removed in the PR (line 90 in old hunk shows assert value != null). The explicit null check with exception is more appropriate for production code than assertions, which can be disabled. The suggestion's premise is outdated.	Low

---

Previous suggestions

Suggestions up to commit e734c19

Category	Suggestion	Impact
General	Add timeout to assertBusy call The assertBusy block waits for the cluster health to become RED or YELLOW, but does not specify a timeout. If the cluster never transitions to the expected state (e.g., due to a bug or test environment issue), the test will hang indefinitely. Add an explicit timeout to assertBusy to ensure the test fails fast if the condition is not met within a reasonable time. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/CompositeFieldCapabilityIT.java [795-801] -public void testMappingUpdateFailsWithUnsupportedField() throws Exception { - ... - // The shard should become unhealthy as the mapping update fails on the data node. - // Wait for the cluster to detect the failure. - assertBusy(() -> { - String health = client().admin().cluster().prepareHealth(indexName).get().getStatus().name(); - assertTrue( - "Index should be RED or YELLOW after unsupported mapping update, got: " + health, - health.equals("RED") || health.equals("YELLOW") - ); - }); +assertBusy(() -> { + String health = client().admin().cluster().prepareHealth(indexName).get().getStatus().name(); + assertTrue( + "Index should be RED or YELLOW after unsupported mapping update, got: " + health, + health.equals("RED") || health.equals("YELLOW") + ); +}, 30, TimeUnit.SECONDS); Suggestion importance[1-10]: 8 __ Why: The suggestion identifies a critical issue where assertBusy without a timeout could cause the test to hang indefinitely if the expected cluster state is never reached. Adding an explicit timeout ensures the test fails fast and provides better test reliability.	Medium
	Verify key existence before assertion The assertion logic assumes that when a keyword value exceeds ignore_above, the main field is null and the raw value is stored in _ignored_source.field. However, the test does not verify that the _ignored_source.field key actually exists before calling get(). If the key is missing, get() returns null, and the assertion will fail with a misleading message. Add an explicit check that the key exists before asserting its value. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/CompositeFieldCapabilityIT.java [498-503] -public void testKeywordIgnoreAboveIndexAndVerify() throws Exception { - ... - // doc2: value exceeds ignore_above — field is null (ignored), source keyword stores raw value - assertNull("field should be null for doc2 (exceeds ignore_above)", doc2Row.get("field")); - assertEquals( - "sourceKeywordFieldType should store raw value for doc2", - "this_exceeds_ignore_above_limit", - doc2Row.get("_ignored_source.field") - ); +assertNull("field should be null for doc2 (exceeds ignore_above)", doc2Row.get("field")); +assertTrue("_ignored_source.field should exist for doc2", doc2Row.containsKey("_ignored_source.field")); +assertEquals( + "sourceKeywordFieldType should store raw value for doc2", + "this_exceeds_ignore_above_limit", + doc2Row.get("_ignored_source.field") +); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential issue where doc2Row.get("_ignored_source.field") could return null if the key doesn't exist, leading to a misleading assertion failure. Adding an explicit containsKey check improves test robustness and provides clearer failure messages.	Medium
	Reorder null and capability checks The null check should occur before checking capabilities to avoid misleading error messages. If a field has no capabilities (empty set), the current code throws a null value error even when the actual issue is missing capability support. Reorder the checks so capability validation happens first. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/index/LuceneDocumentInput.java [90-99] +if (capabilities.isEmpty()) { + // nothing to support on this format for this field. + return; +} if (value == null) { throw new IllegalArgumentException( "Field value must not be null for: " + fieldType.name() + " of type: " + fieldType.typeName() ); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a logical ordering issue. When capabilities is empty, the field is skipped, but if value is null, an error is thrown first. Reordering ensures that fields with no capabilities are filtered out before null validation, avoiding misleading error messages. However, the impact is moderate since null values are typically caught during parsing.	Medium
	Update error message for clarity The error message only mentions INDEX_DERIVED_SOURCE_SETTING but the condition now also checks PLUGGABLE_DATAFORMAT_ENABLED_SETTING. Update the error message to reflect both settings that can trigger this validation failure. server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java [187-192] if ((context.indexSettings().getAsBoolean(IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey(), false) || context.indexSettings().getAsBoolean(IndexSettings.PLUGGABLE_DATAFORMAT_ENABLED_SETTING.getKey(), false)) && !enabled.getValue()) { throw new MapperParsingException( - "_source can't be disabled with " + IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey() + " enabled index setting" + "_source can't be disabled when " + IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey() + " or " + IndexSettings.PLUGGABLE_DATAFORMAT_ENABLED_SETTING.getKey() + " is enabled" ); Suggestion importance[1-10]: 7 __ Why: The error message should accurately reflect both conditions that can trigger the validation failure. Currently it only mentions INDEX_DERIVED_SOURCE_SETTING but the condition now checks both INDEX_DERIVED_SOURCE_SETTING and PLUGGABLE_DATAFORMAT_ENABLED_SETTING. This is a valid improvement for user clarity.	Medium
	Validate field name before concatenation The method constructs a field name by concatenating "_ignored_source." with fieldType().name(). If fieldType().name() contains special characters or is null, this could produce an invalid or unexpected field name. Validate that fieldType().name() is non-null and does not contain problematic characters before constructing the field name. server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java [887-892] private KeywordFieldType buildRawKeywordValueFieldType() { if (isIneligibleForGeneratingSource() && canConsumeRawValueForSource) { - return new KeywordFieldType("_ignored_source." + fieldType().name(), false, true, false, false, fieldType().meta()); + String fieldName = fieldType().name(); + if (fieldName == null || fieldName.isEmpty()) { + throw new IllegalStateException("Field name cannot be null or empty when building raw keyword value field type"); + } + return new KeywordFieldType("_ignored_source." + fieldName, false, true, false, false, fieldType().meta()); } return null; } Suggestion importance[1-10]: 6 __ Why: While the suggestion to validate fieldType().name() is reasonable for defensive programming, it's unlikely that fieldType().name() would be null or empty in practice given the context. However, adding this validation would improve code robustness and provide clearer error messages if such a scenario occurs.	Low
	Improve error message clarity When no formats are configured, the error message lists an empty format list which is confusing. Add an early check to provide a clearer error message when the formats list is empty, indicating that no data formats are configured for the index. sandbox/plugins/composite-engine/src/main/java/org/opensearch/composite/CompositeDataFormatPlugin.java [396-409] if (remaining.isEmpty() == false) { + String formatsList = formats.isEmpty() + ? "none configured" + : formats.stream().map(DataFormat::name).collect(Collectors.toList()).toString(); throw new MapperParsingException( "Field [" + fieldType.name() + "] of type [" + typeName + "] requires capabilities " + requested + " but configured data formats cannot collectively cover: " + remaining + ". Configured formats: " - + formats.stream().map(DataFormat::name).collect(Collectors.toList()) + + formatsList ); } Suggestion importance[1-10]: 6 __ Why: The suggestion improves error message clarity when no formats are configured by replacing an empty list with "none configured". This is a minor usability improvement that helps users understand the root cause more quickly, but does not fix a functional bug.	Low
	Prevent duplicate capability assignments The capability assignment in the constructor bypasses the Builder path and may assign capabilities to mappers that were already processed during the Builder phase. This could result in duplicate capability assignments or inconsistent state. Consider tracking which mappers have already been assigned capabilities to avoid redundant processing. server/src/main/java/org/opensearch/index/mapper/DocumentMapper.java [227-235] final DataFormatRegistry registry = mapperService.documentMapperParser().getDataFormatRegistry(); if (indexSettings.isPluggableDataFormatEnabled() && registry != null) { - assignCapabilitiesRecursive(mapping.root(), registry, indexSettings); - for (MetadataFieldMapper metadataMapper : mapping.metadataMappers) { - registry.assignCapabilities(metadataMapper.fieldType(), indexSettings); + // Only assign capabilities if this mapper was created via merge (bypassing Builder) + if (version > 0) { + assignCapabilitiesRecursive(mapping.root(), registry, indexSettings); + for (MetadataFieldMapper metadataMapper : mapping.metadataMappers) { + registry.assignCapabilities(metadataMapper.fieldType(), indexSettings); + } } } Suggestion importance[1-10]: 4 __ Why: The suggestion attempts to prevent duplicate capability assignments by checking version > 0, but this logic is flawed. The version field is incremented on each merge, so checking version > 0 would skip capability assignment for the initial mapper (version 0), which is incorrect. The PR already handles this by assigning capabilities in both the Builder path and the merge path, ensuring all mappers are covered. The suggestion introduces a bug rather than fixing one.	Low

Suggestions up to commit b536cfb

Category	Suggestion	Impact
Possible issue	Fix incorrect package declaration The package declaration does not match the file path. The file is located in org.opensearch.be.lucene.index but declares package org.opensearch.index.engine.dataformat. This will cause compilation failures. Update the package declaration to match the actual directory structure. sandbox/plugins/analytics-backend-lucene/src/test/java/org/opensearch/be/lucene/index/DeleterImplTests.java [9] -package org.opensearch.index.engine.dataformat; +package org.opensearch.be.lucene.index; import org.opensearch.test.OpenSearchTestCase; Suggestion importance[1-10]: 10 __ Why: The package declaration org.opensearch.index.engine.dataformat does not match the file path org.opensearch.be.lucene.index, which will cause compilation failures. This is a critical error that must be fixed.	High
General	Fix contradictory assertion message The assertion message states the field "should be present" but the assertion checks for null (absence). This is contradictory and will confuse developers. Either fix the message to match the assertion or fix the assertion to match the intended behavior. sandbox/plugins/analytics-backend-lucene/src/test/java/org/opensearch/be/lucene/index/LuceneDocumentInputTests.java [106] public void testSeqNoFieldProperties() { MappedFieldType seqNoField = mock(MappedFieldType.class); when(seqNoField.typeName()).thenReturn(SeqNoFieldMapper.CONTENT_TYPE); when(seqNoField.name()).thenReturn(SeqNoFieldMapper.NAME); Document doc = input.getFinalInput(); IndexableField field = doc.getField(SeqNoFieldMapper.NAME); - assertNull("_seq_no field should be present in document", field); + assertNull("_seq_no field should not be present in document", field); } Suggestion importance[1-10]: 9 __ Why: The assertion message "should be present" contradicts the assertNull check. This is a critical bug in the test that will confuse developers. The message should say "should not be present" to match the assertion.	High
	Verify correct IndexOptions constant The assertion checks that indexOptions is not equal to IndexOptions.DOCS, but the original code checked against IndexOptions.NONE. This change may break the test's intent. Verify that DOCS is the correct value to check against, or revert to NONE if the field should have inverted index options. sandbox/plugins/analytics-backend-lucene/src/test/java/org/opensearch/be/lucene/index/LuceneDocumentInputTests.java [96] -assertNotEquals("match_only_text: should be indexed", IndexOptions.DOCS, ft.indexOptions()); +assertNotEquals("match_only_text: should be indexed", IndexOptions.NONE, ft.indexOptions()); Suggestion importance[1-10]: 8 __ Why: The change from IndexOptions.NONE to IndexOptions.DOCS may alter the test's intent. The suggestion correctly flags this as needing verification, as the original test checked that the field was indexed (not NONE), but the new code checks it's not DOCS.	Medium
	Add null check before assertion The test assumes _ignored_source.field will always be present when field is null due to ignore_above. However, if the storage layer implementation changes or the field is missing, doc2Row.get("_ignored_source.field") returns null, causing a confusing assertion failure. Add an explicit null check before the equality assertion. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/CompositeFieldCapabilityIT.java [498-503] assertNull("field should be null for doc2 (exceeds ignore_above)", doc2Row.get("field")); +assertNotNull("_ignored_source.field should be present for doc2", doc2Row.get("_ignored_source.field")); assertEquals( "sourceKeywordFieldType should store raw value for doc2", "this_exceeds_ignore_above_limit", doc2Row.get("_ignored_source.field") ); Suggestion importance[1-10]: 7 __ Why: Adding an explicit null check for _ignored_source.field before the equality assertion makes the test more robust and provides clearer failure messages if the field is unexpectedly missing.	Medium
	Reorder capability and null checks The null check should occur before checking capabilities to avoid misleading error messages. If a field has no capabilities (empty set), the current code throws a null value error even when the actual issue is missing capability support. Reorder the checks so capability validation happens first, then null validation only for fields that should be processed. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/index/LuceneDocumentInput.java [90-95] +Set<FieldTypeCapabilities.Capability> capabilities = fieldType.getCapabilityMap().getOrDefault(LucenePlugin.DATA_FORMAT, Set.of()); +if (capabilities.isEmpty()) { + return; +} if (value == null) { throw new IllegalArgumentException( "Field value must not be null for: " + fieldType.name() + " of type: " + fieldType.typeName() ); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the null check should occur after the capability check to avoid misleading error messages. However, the current code already performs the capability check first (line 90-93), so the suggested reordering is already implemented in the PR.	Medium
	Update error message for clarity The error message only mentions INDEX_DERIVED_SOURCE_SETTING but the condition now also checks PLUGGABLE_DATAFORMAT_ENABLED_SETTING. Update the error message to reflect both settings that can trigger this validation failure. server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java [187-192] if ((context.indexSettings().getAsBoolean(IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey(), false) || context.indexSettings().getAsBoolean(IndexSettings.PLUGGABLE_DATAFORMAT_ENABLED_SETTING.getKey(), false)) && !enabled.getValue()) { throw new MapperParsingException( - "_source can't be disabled with " + IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey() + " enabled index setting" + "_source can't be disabled when either " + IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey() + + " or " + IndexSettings.PLUGGABLE_DATAFORMAT_ENABLED_SETTING.getKey() + " is enabled" ); Suggestion importance[1-10]: 7 __ Why: The error message should accurately reflect both conditions that can trigger the validation failure. Currently, it only mentions INDEX_DERIVED_SOURCE_SETTING but the condition now also checks PLUGGABLE_DATAFORMAT_ENABLED_SETTING. This is a valid improvement for user clarity and debugging.	Medium
	Add descriptive error messages Using orElseThrow() without a custom exception message makes debugging harder when the expected row is not found. Provide a descriptive error message to indicate which document ID was missing from the parquet rows. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/CompositeFieldCapabilityIT.java [491-492] -Map<String, Object> doc1Row = parquetRows.stream().filter(r -> "doc1".equals(r.get("id_field"))).findFirst().orElseThrow(); -Map<String, Object> doc2Row = parquetRows.stream().filter(r -> "doc2".equals(r.get("id_field"))).findFirst().orElseThrow(); +Map<String, Object> doc1Row = parquetRows.stream() + .filter(r -> "doc1".equals(r.get("id_field"))) + .findFirst() + .orElseThrow(() -> new AssertionError("Expected row with id_field='doc1' not found in parquet")); +Map<String, Object> doc2Row = parquetRows.stream() + .filter(r -> "doc2".equals(r.get("id_field"))) + .findFirst() + .orElseThrow(() -> new AssertionError("Expected row with id_field='doc2' not found in parquet")); Suggestion importance[1-10]: 6 __ Why: Adding descriptive error messages to orElseThrow() improves debuggability when assertions fail. This is a helpful improvement but not critical since the test will still fail with a stack trace.	Low
	Document capability assignment timing The capability assignment for dynamically merged mappers happens in the constructor after field mappers are already built. This may cause race conditions or inconsistent state if mappers are accessed before capabilities are assigned. Consider moving this logic earlier in the build process or document why post-construction assignment is safe. server/src/main/java/org/opensearch/index/mapper/DocumentMapper.java [229-235] +// Assign capabilities immediately after mapper construction to ensure consistency if (indexSettings.isPluggableDataFormatEnabled() && registry != null) { assignCapabilitiesRecursive(mapping.root(), registry, indexSettings); for (MetadataFieldMapper metadataMapper : mapping.metadataMappers) { registry.assignCapabilities(metadataMapper.fieldType(), indexSettings); } } Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern about capability assignment timing for dynamically merged mappers. However, the comment alone doesn't address the potential race condition. The code may be safe if mappers aren't accessed before construction completes, but this should be verified.	Low
	Cache row objects to reduce lookups Repeatedly calling rows.get(i - 1) and rows.get(i) inside the loop is inefficient. Cache the previous and current row objects outside the inner conditional blocks to reduce redundant map lookups and improve readability. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/AbstractSortedRefreshIT.java [144-169] for (int i = 1; i < rows.size(); i++) { - Object prevAge = rows.get(i - 1).get("age"); - Object currAge = rows.get(i).get("age"); + Map<String, Object> prevRow = rows.get(i - 1); + Map<String, Object> currRow = rows.get(i); + Object prevAge = prevRow.get("age"); + Object currAge = currRow.get("age"); ... if (prevAgeVal == currAgeVal) { - Object prevName = rows.get(i - 1).get("name"); - Object currName = rows.get(i).get("name"); + Object prevName = prevRow.get("name"); + Object currName = currRow.get("name"); Suggestion importance[1-10]: 5 __ Why: Caching row objects reduces redundant map lookups and improves code readability. This is a minor optimization that enhances maintainability but does not fix a bug or address a critical issue.	Low

Suggestions up to commit 9d26779

Category	Suggestion	Impact
General	Refine raw value storage condition The condition textValue.length() > ignoreAbove may cause the raw value to be stored even when value is null (i.e., when the field is ignored). This could lead to inconsistent behavior where the main field is absent but the raw source field is present. Consider only storing the raw value when value is non-null or when normalization occurs. server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java [936-950] @Override protected void parseCreateFieldForPluggableFormat(ParseContext context) throws IOException { String textValue = textValue(context); if (textValue == null) { return; } String value = parseKeyword(textValue); + boolean fieldIgnored = (value == null && textValue.length() > ignoreAbove); + boolean normalized = !Objects.equals(normalizerName, "default"); + if (value != null) { context.documentInput().addField(fieldType(), value); } - // For derived source: store raw value separately when normalizer/ignore_above alters it. - // Skip for multi-field sub-fields (name contains dot) — parent field stores the raw source. - if (rawKeywordValueFieldType != null && (textValue.length() > ignoreAbove || !Objects.equals(normalizerName, "default"))) { + // Store raw value when field is ignored OR normalized (for source derivation) + if (rawKeywordValueFieldType != null && (fieldIgnored || normalized)) { context.documentInput().addField(rawKeywordValueFieldType, textValue); } } Suggestion importance[1-10]: 8 __ Why: The suggestion identifies a potential inconsistency where the raw value might be stored even when the main field is null (ignored). The improved code makes the logic clearer by explicitly checking for both ignored and normalized cases, which could prevent subtle bugs in source derivation.	Medium
	Prevent capability map re-assignment The capability map should only be set once during mapping construction. Add a guard to prevent accidental re-assignment which could lead to inconsistent state across concurrent readers. server/src/main/java/org/opensearch/index/mapper/MappedFieldType.java [493-496] @ExperimentalApi public synchronized void setCapabilityMap(Map<DataFormat, Set<FieldTypeCapabilities.Capability>> capabilityMap) { + if (!this.capabilityMap.isEmpty()) { + throw new IllegalStateException("Capability map already set for field: " + name()); + } this.capabilityMap = Map.copyOf(capabilityMap); } Suggestion importance[1-10]: 8 __ Why: This is a valuable safety improvement. The capability map should only be set once during mapping construction, and preventing re-assignment helps catch programming errors early. The guard adds important defensive programming without significant overhead.	Medium
	Check deleteDoc return value The delete thread does not check if deleteDoc returns null after deactivation, which may cause the test to miss a race condition where deletes are silently ignored. Verify the return value and count successful vs. null results to ensure the test accurately detects concurrency issues. sandbox/plugins/analytics-backend-lucene/src/test/java/org/opensearch/be/lucene/index/DeleterImplTests.java [329-340] -public void testConcurrentDeleteAndDeactivate() throws Exception { - ... - Future<?> deleteFuture = executor.submit(() -> { - try { - barrier.await(); - for (int i = 0; i < 100; i++) { - DeleteInput deleteInput = new DeleteInput("_id", "doc" + i, 1L); - deleter.deleteDoc(deleteInput); - Thread.yield(); +Future<?> deleteFuture = executor.submit(() -> { + try { + barrier.await(); + for (int i = 0; i < 100; i++) { + DeleteInput deleteInput = new DeleteInput("_id", "doc" + i, 1L); + DeleteResult result = deleter.deleteDoc(deleteInput); + if (result == null) { + // Deleter became inactive, stop attempting deletes + break; } - } catch (Exception e) { - exception.set(e); + Thread.yield(); } - }); + } catch (Exception e) { + exception.set(e); + } +}); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the delete thread should check for null return values after deactivation to properly detect race conditions. This is a valid improvement that would make the test more robust in detecting concurrency issues.	Medium
	Reorder null check before capability check The null value check should occur before checking capabilities to prevent misleading error messages. If a field has capabilities but receives a null value, the current order throws an exception about null values even when the field might not be supported by this format. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/index/LuceneDocumentInput.java [89-109] public void addField(MappedFieldType fieldType, Object value) { + if (value == null) { + throw new IllegalArgumentException( + "Field value must not be null for: " + fieldType.name() + " of type: " + fieldType.typeName() + ); + } Set<FieldTypeCapabilities.Capability> capabilities = fieldType.getCapabilityMap().getOrDefault(LucenePlugin.DATA_FORMAT, Set.of()); if (capabilities.isEmpty()) { // nothing to support on this format for this field. return; - } - if (value == null) { - throw new IllegalArgumentException( - "Field value must not be null for: " + fieldType.name() + " of type: " + fieldType.typeName() - ); } LuceneFieldFactory factory = fieldFactory(fieldType); if (factory == null) { // capabilities need to be supported but actual implementation to support lucene field type does not exist. throw new IllegalArgumentException( "Field: " + fieldType.name() + " requests capability: " + capabilities + " but does not have any factory to support" ); } FieldType luceneFieldType = getFieldType(fieldType, capabilities); factory.addField(document, fieldType, value, luceneFieldType); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that null validation should occur before capability checks to provide clearer error messages. However, the current order is functionally correct and the improvement is primarily about error message clarity rather than correctness.	Medium
	Update error message for clarity The error message is misleading when PLUGGABLE_DATAFORMAT_ENABLED_SETTING is true. Update the exception message to reflect both conditions that prevent disabling _source, or make it generic to avoid confusion. server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java [190-192] throw new MapperParsingException( - "_source can't be disabled with " + IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey() + " enabled index setting" + "_source can't be disabled when derived source or pluggable data format is enabled" ); Suggestion importance[1-10]: 7 __ Why: The error message only mentions INDEX_DERIVED_SOURCE_SETTING but the condition now also checks PLUGGABLE_DATAFORMAT_ENABLED_SETTING. The message should reflect both conditions to avoid user confusion. However, this is a user-facing message improvement rather than a critical bug fix.	Medium
	Verify null field assumption The test assumes field is null when ignore_above is exceeded, but the actual behavior may store the normalized value or handle it differently. Verify that the parquet writer implementation actually sets field to null in this scenario, or adjust the assertion to match the actual contract. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/CompositeFieldCapabilityIT.java [498-503] -public void testKeywordIgnoreAboveIndexAndVerify() throws Exception { - ... - // doc2: value exceeds ignore_above — field is null (ignored), source keyword stores raw value - assertNull("field should be null for doc2 (exceeds ignore_above)", doc2Row.get("field")); - assertEquals( - "sourceKeywordFieldType should store raw value for doc2", - "this_exceeds_ignore_above_limit", - doc2Row.get("_ignored_source.field") - ); +// doc2: value exceeds ignore_above — verify actual behavior matches contract +Object doc2Field = doc2Row.get("field"); +// Adjust assertion based on actual parquet writer behavior: +// If field is stored as null when exceeding ignore_above: +assertNull("field should be null for doc2 (exceeds ignore_above)", doc2Field); +// OR if field stores the raw value: +// assertEquals("this_exceeds_ignore_above_limit", doc2Field); +assertEquals( + "sourceKeywordFieldType should store raw value for doc2", + "this_exceeds_ignore_above_limit", + doc2Row.get("_ignored_source.field") +); Suggestion importance[1-10]: 6 __ Why: The test assumes field is null when ignore_above is exceeded, but this assumption should be verified against the actual parquet writer implementation. The suggestion correctly identifies a potential mismatch between test expectations and actual behavior, though the impact is moderate since it's a test verification issue.	Low
	Validate null values before capability check The null value check should occur before capability filtering to ensure consistent error handling. The current order may skip null validation for fields without capabilities, leading to inconsistent behavior. sandbox/plugins/parquet-data-format/src/main/java/org/opensearch/parquet/writer/ParquetDocumentInput.java [43-58] @Override public void addField(MappedFieldType fieldType, Object value) { ensureOpen(); + if (value == null) { + throw new IllegalArgumentException( + "Field value must not be null for: " + fieldType.name() + " of type: " + fieldType.typeName() + ); + } 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; } - if (value == null) { - throw new IllegalArgumentException( - "Field value must not be null for: " + fieldType.name() + " of type: " + fieldType.typeName() - ); - } collectedFields.add(new FieldValuePair(fieldType, value)); } Suggestion importance[1-10]: 6 __ Why: Similar to suggestion 1, this improves error message clarity by validating null values before capability filtering. The current implementation may skip null validation for unsupported fields, though this is a minor inconsistency rather than a critical bug.	Low
	Verify boolean parameter change impact Changing the fourth parameter from false to true alters the field's behavior. Verify that this change is intentional and that all dependent code correctly handles this new configuration, as it may affect field storage or retrieval. server/src/main/java/org/opensearch/index/engine/exec/PrimaryTermFieldType.java [40] -super(SeqNoFieldMapper.PRIMARY_TERM_NAME, false, false, true, TextSearchInfo.NONE, Map.of()); +super(SeqNoFieldMapper.PRIMARY_TERM_NAME, false, false, false, TextSearchInfo.NONE, Map.of()); Suggestion importance[1-10]: 6 __ Why: The suggestion asks to verify the intentional change of the fourth boolean parameter from false to true in the PrimaryTermFieldType constructor. This is a valid concern as changing constructor parameters can have significant implications, but the suggestion incorrectly proposes reverting the change rather than verifying it was intentional.	Low
	Clarify mapping update acceptance The test expects the shard to become unhealthy (RED/YELLOW) after an unsupported mapping update, but does not verify that the mapping update itself was accepted by the cluster-manager. If the update is rejected immediately, the shard may remain GREEN. Add an assertion or comment clarifying the expected cluster-manager behavior before checking shard health. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/CompositeFieldCapabilityIT.java [787-801] -public void testMappingUpdateFailsWithUnsupportedField() throws Exception { - ... - client().admin() - .indices() - .preparePutMapping(indexName) - .setSource("{\"properties\":{\"unsupported_geo\":{\"type\":\"geo_point\"}}}", MediaTypeRegistry.JSON) - .get(); +// Mapping update is accepted by cluster-manager but fails on data node during application +client().admin() + .indices() + .preparePutMapping(indexName) + .setSource("{\"properties\":{\"unsupported_geo\":{\"type\":\"geo_point\"}}}", MediaTypeRegistry.JSON) + .get(); - // The shard should become unhealthy as the mapping update fails on the data node. - assertBusy(() -> { - String health = client().admin().cluster().prepareHealth(indexName).get().getStatus().name(); - assertTrue( - "Index should be RED or YELLOW after unsupported mapping update, got: " + health, - health.equals("RED") || health.equals("YELLOW") - ); - }); +// The shard should become unhealthy as the mapping update fails on the data node. +assertBusy(() -> { + String health = client().admin().cluster().prepareHealth(indexName).get().getStatus().name(); + assertTrue( + "Index should be RED or YELLOW after unsupported mapping update, got: " + health, + health.equals("RED") || health.equals("YELLOW") + ); +}); Suggestion importance[1-10]: 5 __ Why: The suggestion to clarify the expected cluster-manager behavior before checking shard health is reasonable, as it would make the test's assumptions more explicit. However, the test already includes a comment explaining the expected behavior, so the improvement is minor.	Low

Suggestions up to commit b4f6c48

Category	Suggestion	Impact
General	Add timeout to assertBusy call The test expects the shard to become unhealthy after an unsupported mapping update, but does not specify a timeout for assertBusy. This could cause the test to hang indefinitely if the expected state change does not occur. Add an explicit timeout to assertBusy to prevent test hangs and provide clearer failure diagnostics. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/CompositeFieldCapabilityIT.java [795-801] -public void testMappingUpdateFailsWithUnsupportedField() throws Exception { - startCluster(); - String indexName = "test-mapping-update-fail"; - assertIndexCreationSucceeds(indexName, "field", "type=keyword"); +// Wait for the cluster to detect the failure with explicit timeout +assertBusy(() -> { + String health = client().admin().cluster().prepareHealth(indexName).get().getStatus().name(); + assertTrue( + "Index should be RED or YELLOW after unsupported mapping update, got: " + health, + health.equals("RED") || health.equals("YELLOW") + ); +}, 30, TimeUnit.SECONDS); - // Index a document successfully - assertEquals(RestStatus.CREATED, client().prepareIndex(indexName).setId("1").setSource("field", "value1").get().status()); - - client().admin().indices().prepareRefresh(indexName).get(); - - // Verify doc was indexed - assertDocCount(indexName, 1); - - // Attempt to update mapping with an unsupported field type (geo_point). - // The cluster-manager accepts the mapping update, but the data node fails - // when trying to apply it (capability assignment fails for geo_point). - client().admin() - .indices() - .preparePutMapping(indexName) - .setSource("{\"properties\":{\"unsupported_geo\":{\"type\":\"geo_point\"}}}", MediaTypeRegistry.JSON) - .get(); - - // The shard should become unhealthy as the mapping update fails on the data node. - // Wait for the cluster to detect the failure. - assertBusy(() -> { - String health = client().admin().cluster().prepareHealth(indexName).get().getStatus().name(); - assertTrue( - "Index should be RED or YELLOW after unsupported mapping update, got: " + health, - health.equals("RED") || health.equals("YELLOW") - ); - }); -} - Suggestion importance[1-10]: 7 __ Why: Adding an explicit timeout to assertBusy prevents potential test hangs and improves test reliability. This is a valuable improvement for test stability.	Medium
	Update error message for clarity The error message only mentions INDEX_DERIVED_SOURCE_SETTING but the condition now also checks PLUGGABLE_DATAFORMAT_ENABLED_SETTING. Update the error message to reflect both settings that can trigger this validation failure. server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java [187-192] if ((context.indexSettings().getAsBoolean(IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey(), false) || context.indexSettings().getAsBoolean(IndexSettings.PLUGGABLE_DATAFORMAT_ENABLED_SETTING.getKey(), false)) && !enabled.getValue()) { throw new MapperParsingException( - "_source can't be disabled with " + IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey() + " enabled index setting" + "_source can't be disabled when either " + IndexSettings.INDEX_DERIVED_SOURCE_SETTING.getKey() + + " or " + IndexSe...

[github-actions]

❌ Gradle check result for 3bde29b: 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 022ce52

[github-actions]

❌ Gradle check result for 022ce52: 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]

PR Code Analyzer ❗

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

'Diff too large, requires skip by maintainers after manual review'

---

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.

[Bukhtawar]

Thanks for the changes, overall lgtm

[github-actions]

Persistent review updated to latest commit b536cfb

[github-actions]

❌ Gradle check result for b536cfb: 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 e734c19

[github-actions]

✅ Gradle check result for e734c19: SUCCESS

[github-actions]

Persistent review updated to latest commit 3295038

[github-actions]

✅ Gradle check result for 3295038: SUCCESS