Skip to content

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
@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.

5 participants