Skip to content

Stabilize CalcitePPLConditionBuiltinFunctionIT on the analytics-engine route#5556

Merged
ahkcs merged 1 commit into
opensearch-project:mainfrom
ahkcs:fix-condition-analytics-accumulation
Jun 16, 2026
Merged

Stabilize CalcitePPLConditionBuiltinFunctionIT on the analytics-engine route#5556
ahkcs merged 1 commit into
opensearch-project:mainfrom
ahkcs:fix-condition-analytics-accumulation

Conversation

@ahkcs

@ahkcs ahkcs commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Description

Brings CalcitePPLConditionBuiltinFunctionIT to parity on the analytics-engine route (-Dtests.analytics.parquet_indices=true), where every test-created index is composite/parquet-backed and PPL queries route through the analytics engine (DataFusion). The IT already passes on the v2/Calcite route; this change is test-only.

Root cause 1 — append-only accumulation (fixes 12 of 18 failures)

init() seeds two extra docs (_id 7, _id 8) into state_country_with_null via unconditional raw PUTs. init() runs as @Before, before every test method, and preserveClusterUponCompletion() keeps indices across methods. On the analytics-engine parquet-backed store a PUT with an existing _id appends a new row instead of replacing, so the two docs accumulated one duplicate per method and inflated row counts across the suite (the was counts grow monotonically: 13, 16, 25, 26, 32, 36, 42, 45, 47, 50, 52). The bulk loadIndex(...) calls don't have this problem because they're already isIndexExist-guarded. Fix: capture isIndexExist(...) before loadIndex creates the index, then seed the extra docs only on first creation. End state is identical on the v2/Calcite route.

Root cause 2 — four unsupported behaviors on the route (the remaining 6 failures)

These are genuine analytics-engine behaviors, not test bugs (each verified directly against the route). They're skipped via the assumeNotAnalytics(...) registry (AnalyticsRouteLimitation) introduced in #5551, plus matching excludeTestsMatching entries in integTestRemote so the skip set stays countable in one place. NESTED_FIELDS is reused; three new AnalyticsRouteLimitation constants:

  • STRUCT_PARENT_FIELD — querying an object/struct parent field directly (isnull(aws) / isnotnull(aws) on big5.aws) resolves to FIELD_NOT_FOUND. The route flattens objects into dotted leaf columns (aws.cloudwatch.log_group scans fine) but the struct parent is not a queryable column. Distinct from NESTED_FIELDS: object parents survive in the OpenSearch mapping but still can't be referenced as a whole. (testIsNullWithStruct, testIsNotNullWithStruct)
  • NESTED_FIELDS (reused) — nested_simple.address is type nested, which the route cannot store, so the test infra strips it at index creation ([analytics-engine] Strip AE-unsupported fields from test data and exclude the ITs doomed by it #5541) and the field resolves to FIELD_NOT_FOUND. (testIsNullWithNested, testIsNotNullWithNested)
  • CONCAT_NULL_AS_EMPTY — DataFusion concat treats NULL as an empty string (concat('H', null) = 'H'), whereas v2/Calcite propagates NULL (= null), so the null-name row diverges. (testNullIfWithExpression)
  • EARLIEST_LATEST_NOW_CLOCKearliest('now', utc_timestamp()): the relative-time 'now' and utc_timestamp() resolve to the same instant on the route (now >= nowtrue), but differ on v2/Calcite (false) — a clock-source divergence. (testEarliestWithEval)

Results

Run via :integ-test:integTestRemote against an analytics :run cluster.

Test class Analytics route — before Analytics route — after v2/Calcite route
CalcitePPLConditionBuiltinFunctionIT 6/24 pass, 18 fail 18/18 run, 0 fail (6 excluded) 24/24 pass

On the analytics route the skipped tests are removed by excludeTestsMatching so they don't run; each also carries an in-test assumeNotAnalytics(...) as a safety net. The v2/Calcite route runs all 24 (the gradle excludes apply only to integTestRemote, and assumeNotAnalytics is a no-op when the analytics flag is off).

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff.

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.

@ahkcs ahkcs added the infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. label Jun 16, 2026
@ahkcs ahkcs force-pushed the fix-condition-analytics-accumulation branch from 31593a4 to 9ccd9d8 Compare June 16, 2026 19:34
…e route

init() seeds two extra docs into state_country_with_null via unconditional
raw PUTs. init() runs as @before before every test method, and the
analytics-engine parquet-backed store is append-only on same-_id PUT, so the
docs accumulated a duplicate per method and inflated row counts across the
suite. Guard the seed on a pre-loadIndex isIndexExist check so it runs exactly
once; behavior is unchanged on the v2/Calcite route (same end state).

Skip the six tests that exercise behaviors the analytics-engine route does not
support, using the assumeNotAnalytics(...) registry (AnalyticsRouteLimitation)
plus matching excludeTestsMatching entries in integTestRemote so the skip set
stays countable in one place. NESTED_FIELDS is reused; three new constants:
  - STRUCT_PARENT_FIELD: querying an object/struct parent field directly
    (isnull/isnotnull(aws)) resolves to FIELD_NOT_FOUND — the route flattens
    objects to dotted leaf columns and the parent is not a queryable column.
  - CONCAT_NULL_AS_EMPTY: concat('H', null) = 'H' on the route (DataFusion
    NULL-as-empty) vs null on v2/Calcite (NULL-propagating).
  - EARLIEST_LATEST_NOW_CLOCK: earliest('now', utc_timestamp()) is true on the
    route (same instant) but false on v2 (clock-source divergence).
The two nested tests reuse NESTED_FIELDS (nested fields are stripped at index
creation on this route, opensearch-project#5541).

Results (-Dtests.analytics.parquet_indices=true against the analytics route):
  CalcitePPLConditionBuiltinFunctionIT: 6/24 -> 18/18 run, 0 fail (6 excluded)
v2/Calcite route unchanged: 24/24 pass.

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@ahkcs ahkcs force-pushed the fix-condition-analytics-accumulation branch from 9ccd9d8 to db62766 Compare June 16, 2026 20:40
@ahkcs ahkcs merged commit 4c1165a into opensearch-project:main Jun 16, 2026
40 of 46 checks passed
dai-chen added a commit to dai-chen/sql-1 that referenced this pull request Jun 16, 2026
Rename AnalyticsRouteLimitation to Capability and add a declarative
@RequiresCapability annotation enforced by CapabilityRule (JUnit4) on
SQLIntegTestCase, so both SQL and PPL ITs can gate by capability. Replace
all assumeNotAnalytics(...) calls with method-level @RequiresCapability
(including the cases added by opensearch-project#5555 and opensearch-project#5556). Skip stays AE-route-only.

Verified: ./gradlew :integ-test:compileTestJava and :integ-test:spotlessCheck pass.
Signed-off-by: Chen Dai <daichen@amazon.com>
dai-chen added a commit to dai-chen/sql-1 that referenced this pull request Jun 16, 2026
Rename AnalyticsRouteLimitation to Capability and add a declarative
@RequiresCapability annotation enforced by CapabilityRule (JUnit4),
wired into SQLIntegTestCase so SQL and PPL ITs gate by capability.
Gate logic lives in BackendCapabilities (skips on the analytics-engine
route, which supports none of the defined capabilities today). Replace
all assumeNotAnalytics(...) with @RequiresCapability (incl. opensearch-project#5555/opensearch-project#5556).

Verified: ./gradlew :integ-test:compileTestJava and :integ-test:spotlessCheck pass.
Signed-off-by: Chen Dai <daichen@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants