Skip to content

[Analytic Engine] Skip PUT+DELETE doc-fixture tests on the analytics-engine storage matrix#5476

Open
RyanL1997 wants to merge 1 commit into
opensearch-project:mainfrom
RyanL1997:skip-data-format-aware-tests-on-ae-storage
Open

[Analytic Engine] Skip PUT+DELETE doc-fixture tests on the analytics-engine storage matrix#5476
RyanL1997 wants to merge 1 commit into
opensearch-project:mainfrom
RyanL1997:skip-data-format-aware-tests-on-ae-storage

Conversation

@RyanL1997
Copy link
Copy Markdown
Collaborator

@RyanL1997 RyanL1997 commented May 27, 2026

Description

Bucket # 22 of the AE-storage failure report (8 failures: unsupported_operation_exception — delete operation not supported.) is not a SQL plugin bug. The exception originates in OpenSearch core's storage layer:

  • server/src/main/java/org/opensearch/index/engine/DataFormatAwareEngine.java:691delete(...) throws UnsupportedEncodingException("delete operation not supported.")
  • server/src/main/java/org/opensearch/index/engine/DataFormatAwareEngine.java:774prepareDelete(...) throws UnsupportedOperationException("delete operation not supported.")

DataFormatAwareEngine is the engine that backs analytics-engine indices (parquet / composite). It's append-only by design — per-doc delete is not implemented.

The failing tests are 10 PPL / Calcite integration tests that mutate a shared test index as part of their fixture:

PUT /<index>/_doc/<id>?refresh=true   ← seed a tailored doc for this test
... run query, assert ...
DELETE /<index>/_doc/<id>?refresh=true ← clean up to keep the shared index pristine

This pattern works fine against the default Lucene-backed engine. When the suite is launched with -Dtests.analytics.parquet_indices=true (the AE-storage matrix), the cleanup DELETE trips DataFormatAwareEngine.delete() and the test fails — even though the SQL/PPL behavior under test would otherwise pass.

Fix

Skip these specific tests on the AE-storage matrix using the existing helper PPLIntegTestCase.isAnalyticsParquetIndicesEnabled() paired with Assume.assumeFalse(...). This is the precedent already established in StatsCommandIT (4 sites) and CalciteAnalyticsDatetimeWireFormatIT.init().

Impact

Mode Behavior
Default integTest / integTestRemote (mainline CI) unchanged — assumeFalse(false) is a no-op, tests run and assert as before
-Dtests.analytics.parquet_indices=true (AE-storage matrix) 10 affected tests are skipped (logged with reason); bucket # 22 in the report disappears

No assertion is changed, no fixture is refactored, no Lucene-path coverage is lost.

Out of scope

  • Engine-level support for delete on DataFormatAwareEngine — major design question, not a test-fix concern.
  • Refactoring the affected tests to use per-test indices (drop+recreate instead of per-doc DELETE) — would also fix the issue but is significantly more invasive and unnecessary for clearing the report.

Test plan

  • CI: default integTest / integTestRemote passes (no behavior change on Lucene path).
  • Manual: re-run the AE-storage matrix and confirm bucket # 22 no longer appears.
  • ./gradlew :integ-test:spotlessApply ran clean.
  • ./gradlew :integ-test:compileTestJava ran clean.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 27, 2026

PR Reviewer Guide 🔍

(Review updated until commit 7b05885)

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions
Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Simplify multi-line string concatenation

The assumption message is split across multiple lines using string concatenation,
which can be simplified. Consider using a text block or a single string literal for
better readability and maintainability.

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExpandCommandIT.java [299-302]

 assumeFalse(
-    "Test mutates docs via PUT+DELETE, which DataFormatAwareEngine"
-        + " (analytics-engine storage path) does not support.",
+    "Test mutates docs via PUT+DELETE, which DataFormatAwareEngine (analytics-engine storage path) does not support.",
     isAnalyticsParquetIndicesEnabled());
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies that the string concatenation can be simplified into a single line. However, this is a minor style improvement with minimal impact on code quality or functionality. The existing code is already readable and the change is purely cosmetic.

Low

These 10 PPL/Calcite integration tests mutate the shared test index by
issuing PUT /<idx>/_doc/<id> + DELETE /<idx>/_doc/<id> around the query
under test. When the suite runs with -Dtests.analytics.parquet_indices=true,
test indices are backed by DataFormatAwareEngine, whose delete() and
prepareDelete() deliberately throw "delete operation not supported." —
parquet/composite storage is append-only by design.

The tests pass under the Lucene-backed default. Skipping them via
Assume.assumeFalse(isAnalyticsParquetIndicesEnabled()) follows the
precedent already established in StatsCommandIT and
CalciteAnalyticsDatetimeWireFormatIT, and prevents the spurious bucket-22
"delete operation not supported" failures in the AE-storage report
without changing any assertion or test behavior on the default path.

Signed-off-by: Jialiang Liang <jiallian@amazon.com>
@RyanL1997 RyanL1997 force-pushed the skip-data-format-aware-tests-on-ae-storage branch from 549c546 to 7b05885 Compare May 27, 2026 22:22
@github-actions
Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 7b05885

@RyanL1997 RyanL1997 added maintenance Improves code quality, but not the product testing Related to improving software testing labels May 27, 2026
@RyanL1997 RyanL1997 changed the title Skip PUT+DELETE doc-fixture tests on the analytics-engine storage matrix [Analytic Engine] Skip PUT+DELETE doc-fixture tests on the analytics-engine storage matrix May 27, 2026
@RyanL1997 RyanL1997 marked this pull request as ready for review May 27, 2026 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Improves code quality, but not the product testing Related to improving software testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants