-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Cleanup deprecated query options #13040
Cleanup deprecated query options #13040
Conversation
Jackie-Jiang
commented
May 1, 2024
- Cleanup the deprecated debug options, and old PQL related query options
- Fix the double query parsing in multi-stage engine
- Fix the inconsistent behavior when query parsing fails: do not log or emit metric because it is pure user error
sqlNodeAndOptions.getOptions().get(CommonConstants.Broker.Request.QueryOptionKey.USE_MULTISTAGE_ENGINE))) { | ||
return _multiStageBrokerRequestHandler.handleRequest(request, requesterIdentity, requestContext, httpHeaders); | ||
sqlNodeAndOptions.getOptions().get(Request.QueryOptionKey.USE_MULTISTAGE_ENGINE))) { | ||
return _multiStageBrokerRequestHandler.handleRequest(request, sqlNodeAndOptions, requesterIdentity, |
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 is an important fix. Currently we are not passing the parsed SQL into the multi-stage engine, which results in parsing the query twice
09c971b
to
9847b38
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13040 +/- ##
============================================
+ Coverage 61.75% 62.05% +0.30%
+ Complexity 207 198 -9
============================================
Files 2436 2502 +66
Lines 133233 136656 +3423
Branches 20636 21178 +542
============================================
+ Hits 82274 84800 +2526
- Misses 44911 45558 +647
- Partials 6048 6298 +250
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
...roker/src/main/java/org/apache/pinot/broker/requesthandler/BrokerRequestHandlerDelegate.java
Show resolved
Hide resolved
LGTM, but there are multiple tests failing, so I guess we would need to fix something else |
Find the reason. Currently we only set query option when it is not empty, but the following code assume it is always set. After removing the deprecated options it breaks. Changed the code to always set the query option even if it is empty. |
9847b38
to
6f64de0
Compare
6f64de0
to
d581631
Compare