Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discover] fix PPL to not throw error if aggregation query fails #8992

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

joshuali925
Copy link
Member

@joshuali925 joshuali925 commented Dec 2, 2024

Description

The previous PR #8743 added handleFacetError if date_histogram aggregation failed, because otherwise the code will process the response as data, and calling rawAggs.data.datarows caused NPE.

The problem is that some PPL queries will not have a date_histogram, for example source = table | stats count(), and failure is expected. So it should not throw error there. This PR changes handleFacetError to continue, and renames handleFacetError to throwFacetError.

Issues Resolved

  • skip

Screenshot

Testing the changes

added unit tests and locally tested

Changelog

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link
Contributor

github-actions bot commented Dec 2, 2024

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.89%. Comparing base (b31206a) to head (d181c28).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8992   +/-   ##
=======================================
  Coverage   60.88%   60.89%           
=======================================
  Files        3802     3803    +1     
  Lines       91083    91106   +23     
  Branches    14383    14388    +5     
=======================================
+ Hits        55455    55476   +21     
  Misses      32086    32086           
- Partials     3542     3544    +2     
Flag Coverage Δ
Linux_1 29.02% <ø> (ø)
Linux_2 56.38% <ø> (ø)
Linux_3 37.90% <ø> (ø)
Linux_4 29.02% <100.00%> (+0.02%) ⬆️
Windows_1 29.04% <ø> (ø)
Windows_2 56.34% <ø> (ø)
Windows_3 37.90% <ø> (ø)
Windows_4 29.02% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@sejli sejli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Thanks for the search strategy unit tests as well.

Changelog workflow failing because it's under the wrong section in the description.

Copy link
Contributor

github-actions bot commented Dec 3, 2024

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

@joshuali925 joshuali925 merged commit d5e0087 into opensearch-project:main Dec 3, 2024
72 of 74 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 3, 2024
Signed-off-by: Joshua Li <[email protected]>
(cherry picked from commit d5e0087)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
virajsanghvi pushed a commit that referenced this pull request Dec 3, 2024
…) (#8998)

(cherry picked from commit d5e0087)

Signed-off-by: Joshua Li <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants