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

Cleanup deprecated query options #13040

Merged

Conversation

Jackie-Jiang
Copy link
Contributor

  • 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,
Copy link
Contributor Author

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

@Jackie-Jiang Jackie-Jiang force-pushed the cleanup_deprecated_query_options branch from 09c971b to 9847b38 Compare May 1, 2024 02:04
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2024

Codecov Report

Attention: Patch coverage is 30.43478% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 62.05%. Comparing base (59551e4) to head (d581631).
Report is 392 commits behind head on master.

Files Patch % Lines
...pache/pinot/common/utils/request/RequestUtils.java 0.00% 3 Missing and 2 partials ⚠️
...roker/requesthandler/BaseBrokerRequestHandler.java 42.85% 3 Missing and 1 partial ⚠️
...requesthandler/MultiStageBrokerRequestHandler.java 42.85% 3 Missing and 1 partial ⚠️
...r/requesthandler/BrokerRequestHandlerDelegate.java 0.00% 3 Missing ⚠️
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     
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration <0.01% <0.00%> (-0.01%) ⬇️
integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration2 0.00% <0.00%> (ø)
java-11 27.95% <26.08%> (-33.76%) ⬇️
java-21 62.05% <30.43%> (+0.42%) ⬆️
skip-bytebuffers-false 62.02% <30.43%> (+0.28%) ⬆️
skip-bytebuffers-true 62.04% <30.43%> (+34.31%) ⬆️
temurin 62.05% <30.43%> (+0.30%) ⬆️
unittests 62.04% <30.43%> (+0.30%) ⬆️
unittests1 46.54% <16.66%> (-0.35%) ⬇️
unittests2 27.95% <26.08%> (+0.22%) ⬆️

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.

@gortiz
Copy link
Contributor

gortiz commented May 1, 2024

LGTM, but there are multiple tests failing, so I guess we would need to fix something else

@Jackie-Jiang
Copy link
Contributor Author

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.

@Jackie-Jiang Jackie-Jiang force-pushed the cleanup_deprecated_query_options branch from 9847b38 to 6f64de0 Compare May 1, 2024 19:57
@Jackie-Jiang Jackie-Jiang force-pushed the cleanup_deprecated_query_options branch from 6f64de0 to d581631 Compare May 1, 2024 20:33
@Jackie-Jiang Jackie-Jiang merged commit 076cd40 into apache:master May 1, 2024
22 checks passed
@Jackie-Jiang Jackie-Jiang deleted the cleanup_deprecated_query_options branch May 1, 2024 23:03
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