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

[multistage][bugfix] fix tenant detection issues #10546

Merged
merged 2 commits into from Apr 5, 2023

Conversation

walterddr
Copy link
Contributor

@walterddr walterddr commented Apr 4, 2023

follow up on multi-stage tenant support #10336

  • if query doesn't have table name alias. it doesn't work (it is not a SqlCall but a SqlIdentifier after From)
  • adding test for OrderBy query as well b/c order by comes before SqlFrom
  • make error message more clear.

@walterddr walterddr force-pushed the pr_fix_controller_tenant_lookup branch from c662429 to 259aea7 Compare April 4, 2023 23:27
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2023

Codecov Report

Merging #10546 (137aa1c) into master (513fa31) will increase coverage by 2.49%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##             master   #10546      +/-   ##
============================================
+ Coverage     67.84%   70.34%   +2.49%     
- Complexity     6001     6465     +464     
============================================
  Files          1575     2103     +528     
  Lines         81930   112767   +30837     
  Branches      12870    16979    +4109     
============================================
+ Hits          55586    79323   +23737     
- Misses        22450    27898    +5448     
- Partials       3894     5546    +1652     
Flag Coverage Δ
integration1 24.35% <0.00%> (?)
integration2 24.24% <0.00%> (?)
unittests1 67.85% <100.00%> (+0.01%) ⬆️
unittests2 13.87% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...t/controller/api/resources/PinotQueryResource.java 43.25% <0.00%> (ø)
...org/apache/pinot/sql/parsers/CalciteSqlParser.java 85.68% <100.00%> (+1.66%) ⬆️

... and 781 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@walterddr walterddr marked this pull request as ready for review April 5, 2023 16:10
@walterddr walterddr merged commit 29fd5d9 into apache:master Apr 5, 2023
14 checks passed
@walterddr
Copy link
Contributor Author

walterddr commented Apr 8, 2023

Another issue to follow up with this @ankitsultana and @tibrewalpratik17 is that after this fix the error message for multi-stage query for select * from non_exist_tbl will return cannot find common tenant across all tables: ['non_exist_tbl']

where as the real error should probably be table doesn't exist. Please kindly take a look and see how we can better surface the error message since this is user-facing

@walterddr walterddr changed the title [multistage] fix tenant detection issues [multistage][bugfix] fix tenant detection issues Apr 8, 2023
@Jackie-Jiang Jackie-Jiang added bugfix multi-stage Related to the multi-stage query engine labels Apr 10, 2023
@tibrewalpratik17
Copy link
Contributor

@walterddr raised a fix to improve the error message in case of non-existent table in #10599

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix multi-stage Related to the multi-stage query engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants