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

Ensure Nested Function Falls Back to Legacy Engine Where Not Supported #1549

Merged

Conversation

forestmvey
Copy link
Collaborator

@forestmvey forestmvey commented Apr 18, 2023

Description

Fallback to legacy engine when the nested function is used in the WHERE, GROUP BY, ORDER BY, and HAVING clauses. Support has not been added to the V2 engine for the nested query in these clauses.

Issues Resolved

1548

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • 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.

rows("a"),
rows("a"));
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can run query with nested in where and validate server logs to ensure that it is executed in V1.

dai-chen
dai-chen previously approved these changes Apr 18, 2023
Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! I think we need to merge this to 2.7 by code freeze today.

Any idea why our IT didn't capture this? Because I assume there should be exception thrown from v2 without current fix.

…d ORDER BY clauses don't fallback to legacy engine.

Signed-off-by: forestmvey <[email protected]>
@dai-chen
Copy link
Collaborator

Could you update 2.7 release notes too? Thanks!

@dai-chen dai-chen added bug Something isn't working backport 2.x backport 2.7 labels Apr 19, 2023
@forestmvey
Copy link
Collaborator Author

Could you update 2.7 release notes too? Thanks!

Sure I will make another PR for the release notes.

@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.18%. Comparing base (4e9ca01) to head (43b70c4).
Report is 368 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1549   +/-   ##
=========================================
  Coverage     97.18%   97.18%           
- Complexity     4100     4106    +6     
=========================================
  Files           371      371           
  Lines         10448    10462   +14     
  Branches        703      706    +3     
=========================================
+ Hits          10154    10168   +14     
  Misses          287      287           
  Partials          7        7           
Flag Coverage Δ
sql-engine 97.18% <100.00%> (+<0.01%) ⬆️

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.

@forestmvey
Copy link
Collaborator Author

Legacy tests only run against the the V1 engine when they use the function getSqlRequest which specifies a format of json. This is why it is only executing certain tests against the legacy engine.

QueryIT.java#L1444-L1451
SQLIntegTestCase.java#L203

@dai-chen dai-chen requested a review from Yury-Fridlyand April 19, 2023 00:28
@dai-chen
Copy link
Collaborator

@forestmvey I see. Please get others approved and merge it. We need to merge this and release notes PR today. Thanks!

@Yury-Fridlyand Yury-Fridlyand merged commit 59ef998 into opensearch-project:main Apr 19, 2023
@Yury-Fridlyand Yury-Fridlyand deleted the integ-nested-fallback branch April 19, 2023 00:33
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 19, 2023
…d ORDER BY clauses don't fallback to legacy engine. (#1549)

Signed-off-by: forestmvey <[email protected]>
(cherry picked from commit 59ef998)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 19, 2023
…d ORDER BY clauses don't fallback to legacy engine. (#1549)

Signed-off-by: forestmvey <[email protected]>
(cherry picked from commit 59ef998)
dai-chen pushed a commit that referenced this pull request Apr 19, 2023
…d ORDER BY clauses don't fallback to legacy engine. (#1549) (#1555)

Signed-off-by: forestmvey <[email protected]>
(cherry picked from commit 59ef998)

Co-authored-by: Forest Vey <[email protected]>
Yury-Fridlyand pushed a commit that referenced this pull request Apr 19, 2023
…d ORDER BY clauses don't fallback to legacy engine. (#1549) (#1554)

Signed-off-by: forestmvey <[email protected]>
(cherry picked from commit 59ef998)

Co-authored-by: Forest Vey <[email protected]>
acarbonetto pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Apr 28, 2023
…d ORDER BY clauses don't fallback to legacy engine. (opensearch-project#1549)

Signed-off-by: forestmvey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport 2.7 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants