Ensure Nested Function Falls Back to Legacy Engine Where Not Supported by forestmvey · Pull Request #1549 · opensearch-project/sql · GitHub
Skip to content

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

Merged
Yury-Fridlyand merged 1 commit into
opensearch-project:mainfrom
Bit-Quill:integ-nested-fallback
Apr 19, 2023
Merged

Ensure Nested Function Falls Back to Legacy Engine Where Not Supported#1549
Yury-Fridlyand merged 1 commit into
opensearch-project:mainfrom
Bit-Quill:integ-nested-fallback

Conversation

@forestmvey

@forestmvey forestmvey commented Apr 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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.

Yury-Fridlyand
Yury-Fridlyand reviewed Apr 18, 2023
View reviewed changes
Comment thread integ-test/src/test/java/org/opensearch/sql/sql/NestedIT.java

@Yury-Fridlyand Yury-Fridlyand Apr 18, 2023

Copy link
Copy Markdown
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.

Comment thread core/src/main/java/org/opensearch/sql/analysis/Analyzer.java
dai-chen
dai-chen previously approved these changes Apr 18, 2023
View reviewed changes

@dai-chen dai-chen left a comment

Copy link
Copy Markdown
Collaborator

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.

@forestmvey
Fixing bug where Nested functions used in WHERE, GROUP BY, HAVING, an…
43b70c4 
…d ORDER BY clauses don't fallback to legacy engine.

Signed-off-by: forestmvey <forestv@bitquilltech.com>
@forestmvey forestmvey force-pushed the integ-nested-fallback branch from e574dd2 to 43b70c4 Compare April 19, 2023 00:01
@dai-chen

dai-chen commented Apr 19, 2023

Copy link
Copy Markdown
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

forestmvey commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator Author

Could you update 2.7 release notes too? Thanks!

Sure I will make another PR for the release notes.

@codecov-commenter

codecov-commenter commented Apr 19, 2023 •
edited by codecov Bot
Loading

Copy link
Copy Markdown

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

forestmvey commented Apr 19, 2023

Copy link
Copy Markdown
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

dai-chen commented Apr 19, 2023

Copy link
Copy Markdown
Collaborator

@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 opensearch-trigger-bot Bot mentioned this pull request Apr 19, 2023
Merged
opensearch-trigger-bot Bot pushed a commit that referenced this pull request Apr 19, 2023
@forestmvey @github-actions
Fixing bug where Nested functions used in WHERE, GROUP BY, HAVING, an…
eea56ea 
…d ORDER BY clauses don't fallback to legacy engine. (#1549)

Signed-off-by: forestmvey <forestv@bitquilltech.com>
(cherry picked from commit 59ef998)
@opensearch-trigger-bot opensearch-trigger-bot Bot mentioned this pull request Apr 19, 2023
Merged
opensearch-trigger-bot Bot pushed a commit that referenced this pull request Apr 19, 2023
@forestmvey @github-actions
Fixing bug where Nested functions used in WHERE, GROUP BY, HAVING, an…
c8ad561 
…d ORDER BY clauses don't fallback to legacy engine. (#1549)

Signed-off-by: forestmvey <forestv@bitquilltech.com>
(cherry picked from commit 59ef998)
dai-chen pushed a commit that referenced this pull request Apr 19, 2023
@opensearch-trigger-bot @forestmvey
Fixing bug where Nested functions used in WHERE, GROUP BY, HAVING, an…
8854502 
…d ORDER BY clauses don't fallback to legacy engine. (#1549) (#1555)

Signed-off-by: forestmvey <forestv@bitquilltech.com>
(cherry picked from commit 59ef998)

Co-authored-by: Forest Vey <36905077+forestmvey@users.noreply.github.com>
Yury-Fridlyand pushed a commit that referenced this pull request Apr 19, 2023
@opensearch-trigger-bot @forestmvey
Fixing bug where Nested functions used in WHERE, GROUP BY, HAVING, an…
dfdbc57 
…d ORDER BY clauses don't fallback to legacy engine. (#1549) (#1554)

Signed-off-by: forestmvey <forestv@bitquilltech.com>
(cherry picked from commit 59ef998)

Co-authored-by: Forest Vey <36905077+forestmvey@users.noreply.github.com>
acarbonetto pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Apr 28, 2023
@forestmvey @acarbonetto
Fixing bug where Nested functions used in WHERE, GROUP BY, HAVING, an…
5917e3e 
…d ORDER BY clauses don't fallback to legacy engine. (opensearch-project#1549)

Signed-off-by: forestmvey <forestv@bitquilltech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dai-chen dai-chen dai-chen approved these changes
@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner
@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner
@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner
@kavithacm kavithacm Awaiting requested review from kavithacm
@derek-ho derek-ho Awaiting requested review from derek-ho
@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner
@YANG-DB YANG-DB Awaiting requested review from YANG-DB
@rupal-bq rupal-bq Awaiting requested review from rupal-bq
@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner
@vamsimanohar vamsimanohar Awaiting requested review from vamsimanohar vamsimanohar is a code owner
+2 more reviewers
@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand approved these changes
@MaxKsyunz MaxKsyunz MaxKsyunz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport 2.x backport 2.7 bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@forestmvey @dai-chen @codecov-commenter @Yury-Fridlyand @MaxKsyunz