Skip to content

[BugFix] Add regression tests for disabled object field queries (#4904)#5308

Closed
qianheng-aws wants to merge 1 commit intoopensearch-project:mainfrom
qianheng-aws:issue-4904-disabled-object-regression
Closed

[BugFix] Add regression tests for disabled object field queries (#4904)#5308
qianheng-aws wants to merge 1 commit intoopensearch-project:mainfrom
qianheng-aws:issue-4904-disabled-object-regression

Conversation

@qianheng-aws
Copy link
Copy Markdown
Collaborator

Description

Issue #4904 reported that PPL queries failed when a field mapping has enabled:false. The bug manifested as a RuntimeException: cannot translate call AS($t2, $t3) when the Calcite engine tried to push down a filter on a disabled object's sub-field as a script.

Investigation shows this issue has been resolved on main by the cumulative effect of several improvements:

The Calcite engine now correctly resolves sub-field access on disabled objects (typed as MAP) using ITEM(field, key) expressions, and the filter pushdown handles these expressions without error.

This PR adds YAML REST regression tests to prevent the issue from recurring:

  1. Basic source query on an index with a disabled object field
  2. where clause filtering on a disabled object sub-field
  3. fields and sort commands on disabled object sub-fields with value verification

Related Issues

Resolves #4904

Check List

  • New functionality includes testing
  • Commits signed per DCO (-s)
  • spotlessCheck passed
  • YAML REST tests passed

…search-project#4904)

The bug where PPL queries failed when field mapping has enabled:false
has been resolved by prior improvements to MAP path resolution and
script pushdown handling. This commit adds YAML REST regression tests
to prevent the issue from recurring.

Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
Copy link
Copy Markdown
Collaborator Author

Decision Log

Root Cause: When a field mapping has enabled: false, the object field is represented as a MAP type in Calcite. Sub-field access (e.g., log.a) creates ITEM(log, 'a') expressions. In the original bug, the filter pushdown path serialized this as a script containing AS(ITEM(...), alias) which Calcite's RexToLixTranslator could not compile to Java code, causing a 500 error at the OpenSearch shard level.

Approach: Investigation confirmed the bug is no longer reproducible on main. The fix was a cumulative result of several improvements since December 2025: MAP path resolution (#5198, #5206), script pushdown standardization (#4914), and error handling for malformed field names in disabled objects (#4907). This PR adds YAML REST regression tests covering source, where, fields, and sort commands on disabled object sub-fields.

Alternatives Rejected: Writing a dedicated Calcite PPL unit test was considered but rejected because the SCOTT test schema doesn't support MAP types from enabled: false mappings. The YAML REST test exercises the full integration path including index creation with enabled: false, which is the most realistic test for this scenario.

Pitfalls: The v2 (non-Calcite) engine also handles this case without error — it resolves log.a at runtime by traversing the source document's nested structure. The error was specific to the Calcite engine's script pushdown path.

Things to Watch: The Schema-on-Read RFC (#4984) proposes a more comprehensive solution for schema-on-read data sources. When implemented, it may change how disabled object fields are resolved, and these regression tests should continue to pass.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

PR Reviewer Guide 🔍

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

Missing Value Verification

The "PPL where clause filtering on disabled object sub-field" test only verifies the count of results (total: 1, datarows: 1) but does not verify the actual content of the returned row. It would be more robust to also assert that the returned record has log.a = 3 to confirm the correct document was returned.

"PPL where clause filtering on disabled object sub-field":
  - skip:
      features:
        - headers
        - allowed_warnings
  - do:
      allowed_warnings:
        - 'Loading the fielddata on the _id field is deprecated and will be removed in future versions. If you require sorting or aggregating on this field you should also include the id in the body of your documents, and map this field as a keyword field that has [doc_values] enabled'
      headers:
        Content-Type: 'application/json'
      ppl:
        body:
          query: "source=test_issue_4904 | where log.a = 3"
  - match: { total: 1 }
  - length: { datarows: 1 }
Missing String Filter Test

There is no test for filtering on the string sub-field log.b. The test data includes both string and numeric sub-fields, but only numeric filtering (log.a = 3) is tested. Adding a test for where log.b = 'hello' would improve coverage of the disabled object field handling for different data types.

"PPL where clause filtering on disabled object sub-field":
  - skip:
      features:
        - headers
        - allowed_warnings
  - do:
      allowed_warnings:
        - 'Loading the fielddata on the _id field is deprecated and will be removed in future versions. If you require sorting or aggregating on this field you should also include the id in the body of your documents, and map this field as a keyword field that has [doc_values] enabled'
      headers:
        Content-Type: 'application/json'
      ppl:
        body:
          query: "source=test_issue_4904 | where log.a = 3"
  - match: { total: 1 }
  - length: { datarows: 1 }
Teardown Order

In the teardown section, plugins.calcite.enabled is reset to false before deleting the index. If the index deletion fails, the Calcite setting remains disabled. While this is a minor concern, it may be worth considering whether the setting reset should happen after the index deletion, or whether the teardown should be more resilient to partial failures.

teardown:
  - do:
      query.settings:
        body:
          transient:
            plugins.calcite.enabled: false
  - do:
      indices.delete:
        index: test_issue_4904

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Reset transient setting to null in teardown

The teardown resets plugins.calcite.enabled to false, but if the original cluster
setting was null (not set), setting it to false may leave the cluster in a different
state than before the test. Consider using null to truly reset the transient setting
to its default/unset state.

integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4904.yml [34-43]

 teardown:
   - do:
       query.settings:
         body:
           transient:
-            plugins.calcite.enabled: false
+            plugins.calcite.enabled: null
   - do:
       indices.delete:
         index: test_issue_4904
Suggestion importance[1-10]: 5

__

Why: Setting plugins.calcite.enabled to null instead of false in teardown is a valid concern for proper cluster state restoration, but this is a minor issue that only matters if the setting was previously unset rather than explicitly false.

Low
Explicitly specify sort direction for deterministic results

The sort order is not explicitly specified in the PPL query (sort log.a), which
defaults to ascending. If the default sort order ever changes or is
implementation-dependent, the assertions on datarows.0.0 and datarows.1.0 could
fail. Consider explicitly specifying the sort direction in the query (e.g., sort +
log.a) to make the test deterministic and self-documenting.

integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4904.yml [79-95]

 "PPL fields and sort on disabled object sub-field returns correct values":
   ...
+      query: "source=test_issue_4904 | fields log.a | sort + log.a"
+  - match: { total: 2 }
+  - length: { datarows: 2 }
   - match: { datarows.0.0: 1 }
   - match: { datarows.1.0: 3 }
Suggestion importance[1-10]: 4

__

Why: The suggestion to use sort + log.a for explicit ascending order is reasonable for test clarity, but the existing_code snippet uses ... as a placeholder which doesn't match the actual PR diff, making the improved_code partially inaccurate in its representation.

Low

@qianheng-aws
Copy link
Copy Markdown
Collaborator Author

Closing — the bug was already fixed on main by #5198, #5206, #4914, #4907. Regression tests are not needed as a standalone PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] PPL query failed if field mapping has enabled:false

1 participant