-
Notifications
You must be signed in to change notification settings - Fork 0
Legacy fall back with JSON format #237
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
Conversation
sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java
Outdated
Show resolved
Hide resolved
|
|
||
| if (!isJsonSupported) { | ||
| throw new UnsupportedOperationException( | ||
| "Queries with aggregation and in memory operations (cast, literals, alias, math, etc.)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious what of these are supported in legacy engine with JSON format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of those are supported in legacy because they are pushed down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For slightly better error messaging, throw the errors in JsonSupportAnalyzer and you can say what type of functions are unsupported as the node visitor reaches an unsupported node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made each unsupported node visitor throw the exception in 74f88ae. Thanks!
Codecov Report
@@ Coverage Diff @@
## dev-v2-json-support #237 +/- ##
=========================================================
+ Coverage 98.30% 98.39% +0.09%
- Complexity 3691 3713 +22
=========================================================
Files 344 346 +2
Lines 9125 9164 +39
Branches 585 588 +3
=========================================================
+ Hits 8970 9017 +47
+ Misses 150 142 -8
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java
Outdated
Show resolved
Hide resolved
6e16fcc to
682110f
Compare
Signed-off-by: Margarit Hakobyan <[email protected]>
Signed-off-by: Margarit Hakobyan <[email protected]>
Signed-off-by: Margarit Hakobyan <[email protected]>
Signed-off-by: Margarit Hakobyan <[email protected]>
Signed-off-by: Margarit Hakobyan <[email protected]>
Signed-off-by: Margarit Hakobyan <[email protected]>
Signed-off-by: Margarit Hakobyan <[email protected]>
…format Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
This reverts commit 17684b3.
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
20a17a0 to
f7c5ee6
Compare
…format Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: MaxKsyunz <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
| return parser.root(); | ||
| } | ||
|
|
||
| public boolean containsHints(String query) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Longer term, we want to return hints... not whether the parser contains hints.
Maybe, return the whole list now and check the conditional hintsParser.root().getChildCount() > 1 in the calling function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Changed this in 74f88ae
| import org.opensearch.sql.ast.tree.Filter; | ||
| import org.opensearch.sql.ast.tree.Project; | ||
|
|
||
| public class JsonSupportAnalyzer extends AbstractNodeVisitor<Boolean, Object> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonSupportNodeVisitor? I'm not sure why we call things "analyzers" when they're node visitors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Boolean, Object> doesn't seem correct, but I suppose we don't really have a ned for a context yet, nor a return type (which is usually a Plan or OptimizedTree.
If the purpose of the node visitor is just to find unsupported nodes, then I suppose this is fine but we should include a javadoc comment that details this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Longer term, we may consider using this to return a JSON object or a tree that can be turned into a JSON object.
We can remove the Boolean return type if we throw Exceptions in the node visitors.
If not, we should be returning Boolean types, not boolean types. Example, Boolean.TRUE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| if (!isJsonSupported) { | ||
| throw new UnsupportedOperationException( | ||
| "Queries with aggregation and in memory operations (cast, literals, alias, math, etc.)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For slightly better error messaging, throw the errors in JsonSupportAnalyzer and you can say what type of functions are unsupported as the node visitor reaches an unsupported node.
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
|
|
||
| @Test | ||
| public void canThrowUnsupportedExceptionForFunctionJsonQuery() { | ||
| sqlService.execute( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code is common to all the tests, isn't it? They only differ in query sent.
Can you put it in a utility method that's call by the tests?
You can make it even more compact by creating a parameterized test that takes a list of sql statement to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, changed in e0e2365. Please let me know if you had something else in mind.
MaxKsyunz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there unit tests for JsonSupportVisitor class?
A lot of it is flagged as lacking test coverage.
|
Also, DCO check is failing, these changes will need to be squashed before going to upstream. |
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Added in 232196d |
Thanks for the heads up. I'm hoping to squash and merge and have DCO check pass then. |
Signed-off-by: Guian Gumpac <[email protected]>
| * Unsupported features in V2 are ones the produce results that differ from | ||
| * legacy results. | ||
| */ | ||
| public class JsonSupportVisitor extends AbstractNodeVisitor<Boolean, JsonSupportVisitorContext> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change type to Void and only throw exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in c2cb0f3
GabeFernandez310
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me so far, but I'll wait until you address what we talked about this morning before approving.
Signed-off-by: Guian Gumpac <[email protected]>
Addressed in c2cb0f3 |
Signed-off-by: Guian Gumpac <[email protected]>
| public class SQLService { | ||
|
|
||
| private final SQLSyntaxParser parser; | ||
| private final SQLSyntaxParser parser;sq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
| private final SQLSyntaxParser parser;sq | |
| private final SQLSyntaxParser parser; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Fixed in 9233d7c. Thanks
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
* Implement json support for V2 engine Signed-off-by: Margarit Hakobyan <[email protected]> * Reverted some changes Signed-off-by: Margarit Hakobyan <[email protected]> * Removed some fields Signed-off-by: Margarit Hakobyan <[email protected]> * minor fix Signed-off-by: Margarit Hakobyan <[email protected]> * Added a unit test, cleaned up Signed-off-by: Margarit Hakobyan <[email protected]> * Returning raw OpenSearch response when type is json Signed-off-by: Margarit Hakobyan <[email protected]> * Add an integration test, fix checkstyle errors Signed-off-by: Margarit Hakobyan <[email protected]> * Made new engine fallback to legacy for in memory operations for json format Signed-off-by: Guian Gumpac <[email protected]> * Address build failures Signed-off-by: MaxKsyunz <[email protected]> * Added legacy fall back Signed-off-by: Guian Gumpac <[email protected]> * Refactored fall back logic to use visitor design pattern Signed-off-by: Guian Gumpac <[email protected]> * Added unit tests Signed-off-by: Guian Gumpac <[email protected]> * Removed unnecessary IT Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR feedback Signed-off-by: Guian Gumpac <[email protected]> * Removed unnecessary context Signed-off-by: Guian Gumpac <[email protected]> * Added fall back for Filter functions Signed-off-by: Guian Gumpac <[email protected]> * Made new engine fallback to legacy for in memory operations for json format Signed-off-by: Guian Gumpac <[email protected]> * Address build failures Signed-off-by: MaxKsyunz <[email protected]> * Added legacy fall back Signed-off-by: Guian Gumpac <[email protected]> * Refactored fall back logic to use visitor design pattern Signed-off-by: Guian Gumpac <[email protected]> * Added unit tests Signed-off-by: Guian Gumpac <[email protected]> * Removed unnecessary IT Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR feedback Signed-off-by: Guian Gumpac <[email protected]> * Removed unnecessary context Signed-off-by: Guian Gumpac <[email protected]> * Added fall back for Filter functions Signed-off-by: Guian Gumpac <[email protected]> --------- Signed-off-by: MaxKsyunz <[email protected]> * Fixed checkstyle errors Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR comments and fixed the visitor Signed-off-by: Guian Gumpac <[email protected]> * Added comment to visitor class Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR comments to improve visitor class Signed-off-by: Guian Gumpac <[email protected]> * Added unit tests for JsonSupportVisitor Signed-off-by: Guian Gumpac <[email protected]> * Added helper function for SQLServiceTest Signed-off-by: Guian Gumpac <[email protected]> * Added expected failures Signed-off-by: Guian Gumpac <[email protected]> * Reworked the visitor class to have type Void instead of Boolean Signed-off-by: Guian Gumpac <[email protected]> * Fixed typo Signed-off-by: Guian Gumpac <[email protected]> * Added github link for tracking issue Signed-off-by: Guian Gumpac <[email protected]> --------- Signed-off-by: Margarit Hakobyan <[email protected]> Signed-off-by: Guian Gumpac <[email protected]> Signed-off-by: MaxKsyunz <[email protected]> Co-authored-by: Margarit Hakobyan <[email protected]> Co-authored-by: MaxKsyunz <[email protected]>
* Implement json support for V2 engine Signed-off-by: Margarit Hakobyan <[email protected]> * Reverted some changes Signed-off-by: Margarit Hakobyan <[email protected]> * Removed some fields Signed-off-by: Margarit Hakobyan <[email protected]> * minor fix Signed-off-by: Margarit Hakobyan <[email protected]> * Added a unit test, cleaned up Signed-off-by: Margarit Hakobyan <[email protected]> * Returning raw OpenSearch response when type is json Signed-off-by: Margarit Hakobyan <[email protected]> * Add an integration test, fix checkstyle errors Signed-off-by: Margarit Hakobyan <[email protected]> * Made new engine fallback to legacy for in memory operations for json format Signed-off-by: Guian Gumpac <[email protected]> * Address build failures Signed-off-by: MaxKsyunz <[email protected]> * Added legacy fall back Signed-off-by: Guian Gumpac <[email protected]> * Refactored fall back logic to use visitor design pattern Signed-off-by: Guian Gumpac <[email protected]> * Added unit tests Signed-off-by: Guian Gumpac <[email protected]> * Removed unnecessary IT Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR feedback Signed-off-by: Guian Gumpac <[email protected]> * Removed unnecessary context Signed-off-by: Guian Gumpac <[email protected]> * Added fall back for Filter functions Signed-off-by: Guian Gumpac <[email protected]> * Made new engine fallback to legacy for in memory operations for json format Signed-off-by: Guian Gumpac <[email protected]> * Address build failures Signed-off-by: MaxKsyunz <[email protected]> * Added legacy fall back Signed-off-by: Guian Gumpac <[email protected]> * Refactored fall back logic to use visitor design pattern Signed-off-by: Guian Gumpac <[email protected]> * Added unit tests Signed-off-by: Guian Gumpac <[email protected]> * Removed unnecessary IT Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR feedback Signed-off-by: Guian Gumpac <[email protected]> * Removed unnecessary context Signed-off-by: Guian Gumpac <[email protected]> * Added fall back for Filter functions Signed-off-by: Guian Gumpac <[email protected]> --------- Signed-off-by: MaxKsyunz <[email protected]> * Fixed checkstyle errors Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR comments and fixed the visitor Signed-off-by: Guian Gumpac <[email protected]> * Added comment to visitor class Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR comments to improve visitor class Signed-off-by: Guian Gumpac <[email protected]> * Added unit tests for JsonSupportVisitor Signed-off-by: Guian Gumpac <[email protected]> * Added helper function for SQLServiceTest Signed-off-by: Guian Gumpac <[email protected]> * Added expected failures Signed-off-by: Guian Gumpac <[email protected]> * Reworked the visitor class to have type Void instead of Boolean Signed-off-by: Guian Gumpac <[email protected]> * Fixed typo Signed-off-by: Guian Gumpac <[email protected]> * Added github link for tracking issue Signed-off-by: Guian Gumpac <[email protected]> --------- Signed-off-by: Margarit Hakobyan <[email protected]> Signed-off-by: Guian Gumpac <[email protected]> Signed-off-by: MaxKsyunz <[email protected]> Co-authored-by: Margarit Hakobyan <[email protected]> Co-authored-by: MaxKsyunz <[email protected]>
* Implement json support for V2 engine Signed-off-by: Margarit Hakobyan <[email protected]> * Reverted some changes Signed-off-by: Margarit Hakobyan <[email protected]> * Removed some fields Signed-off-by: Margarit Hakobyan <[email protected]> * minor fix Signed-off-by: Margarit Hakobyan <[email protected]> * Added a unit test, cleaned up Signed-off-by: Margarit Hakobyan <[email protected]> * Returning raw OpenSearch response when type is json Signed-off-by: Margarit Hakobyan <[email protected]> * Add an integration test, fix checkstyle errors Signed-off-by: Margarit Hakobyan <[email protected]> * Added constructor for empty rawResponse Signed-off-by: Guian Gumpac <[email protected]> * Added constant for supported formats Signed-off-by: Guian Gumpac <[email protected]> * Added unit test Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR comments Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR comments: Signed-off-by: Guian Gumpac <[email protected]> * Fixed issue Signed-off-by: Guian Gumpac <[email protected]> * Added getter for rawResponse in PhysicalPlan Signed-off-by: Guian Gumpac <[email protected]> * Legacy fall back with JSON format (#237) * Implement json support for V2 engine Signed-off-by: Margarit Hakobyan <[email protected]> * Reverted some changes Signed-off-by: Margarit Hakobyan <[email protected]> * Removed some fields Signed-off-by: Margarit Hakobyan <[email protected]> * minor fix Signed-off-by: Margarit Hakobyan <[email protected]> * Added a unit test, cleaned up Signed-off-by: Margarit Hakobyan <[email protected]> * Returning raw OpenSearch response when type is json Signed-off-by: Margarit Hakobyan <[email protected]> * Add an integration test, fix checkstyle errors Signed-off-by: Margarit Hakobyan <[email protected]> * Made new engine fallback to legacy for in memory operations for json format Signed-off-by: Guian Gumpac <[email protected]> * Address build failures Signed-off-by: MaxKsyunz <[email protected]> * Added legacy fall back Signed-off-by: Guian Gumpac <[email protected]> * Refactored fall back logic to use visitor design pattern Signed-off-by: Guian Gumpac <[email protected]> * Added unit tests Signed-off-by: Guian Gumpac <[email protected]> * Removed unnecessary IT Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR feedback Signed-off-by: Guian Gumpac <[email protected]> * Removed unnecessary context Signed-off-by: Guian Gumpac <[email protected]> * Added fall back for Filter functions Signed-off-by: Guian Gumpac <[email protected]> * Made new engine fallback to legacy for in memory operations for json format Signed-off-by: Guian Gumpac <[email protected]> * Address build failures Signed-off-by: MaxKsyunz <[email protected]> * Added legacy fall back Signed-off-by: Guian Gumpac <[email protected]> * Refactored fall back logic to use visitor design pattern Signed-off-by: Guian Gumpac <[email protected]> * Added unit tests Signed-off-by: Guian Gumpac <[email protected]> * Removed unnecessary IT Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR feedback Signed-off-by: Guian Gumpac <[email protected]> * Removed unnecessary context Signed-off-by: Guian Gumpac <[email protected]> * Added fall back for Filter functions Signed-off-by: Guian Gumpac <[email protected]> --------- Signed-off-by: MaxKsyunz <[email protected]> * Fixed checkstyle errors Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR comments and fixed the visitor Signed-off-by: Guian Gumpac <[email protected]> * Added comment to visitor class Signed-off-by: Guian Gumpac <[email protected]> * Addressed PR comments to improve visitor class Signed-off-by: Guian Gumpac <[email protected]> * Added unit tests for JsonSupportVisitor Signed-off-by: Guian Gumpac <[email protected]> * Added helper function for SQLServiceTest Signed-off-by: Guian Gumpac <[email protected]> * Added expected failures Signed-off-by: Guian Gumpac <[email protected]> * Reworked the visitor class to have type Void instead of Boolean Signed-off-by: Guian Gumpac <[email protected]> * Fixed typo Signed-off-by: Guian Gumpac <[email protected]> * Added github link for tracking issue Signed-off-by: Guian Gumpac <[email protected]> --------- Signed-off-by: Margarit Hakobyan <[email protected]> Signed-off-by: Guian Gumpac <[email protected]> Signed-off-by: MaxKsyunz <[email protected]> Co-authored-by: Margarit Hakobyan <[email protected]> Co-authored-by: MaxKsyunz <[email protected]> * Reverted OpenSearchIndexScan changes as it broke IT Signed-off-by: Guian Gumpac <[email protected]> * Added unit test Signed-off-by: Guian Gumpac <[email protected]> * Removed unused Mock variable Signed-off-by: Guian Gumpac <[email protected]> --------- Signed-off-by: Margarit Hakobyan <[email protected]> Signed-off-by: Guian Gumpac <[email protected]> Signed-off-by: MaxKsyunz <[email protected]> Co-authored-by: Guian Gumpac <[email protected]> Co-authored-by: MaxKsyunz <[email protected]>
NOTE
Actions are expected to fail as the migration of tests will be part of a separate PR.
Jacoco will fail on OpenSearchExecutionEngine and will be fixed in the parent PR as there may be improvements that can be done.
Description
JSON implementation comes from PR #217.
JSON format for V2 does not support and falls back to legacy for the following:
TODO before being able to merge to upstream main:
DateFormatIT. and
DateFormatIT. equalTo
DateFormatIT. greaterThan
DateFormatIT. greaterThanOrEqualTo
DateFormatIT. lessThan
DateFormatIT. lessThanOrEqualTo
DateFormatIT. or
DateFormatIT. sortByDateFormat
QueryFunctionsIT. wildcardQuery (Dependent on PR #1314)
QueryIT. dateBetweenSearch
QueryIT. dateSearch
QueryIT. selectAllWithFieldAndOrderBy
QueryIT. selectAllWithFieldReturnsAll
QueryIT. selectAllWithFieldReverseOrder
QueryIT. selectAllWithMultipleFields
QueryIT. inTest (mismatch with legacy result due to lack of score function. Can be ignored for now as score implementation is in progress)
QueryIT. notLikeTests (mismatch with legacy result due to lack of score function. Can be ignored for now as score implementation is in progress)
Issues Resolved
opensearch-project#1317
Check List
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.