[Multistage] Allow queries on multiple tables of same tenant to be executed from controller UI #10336
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10336 +/- ##
============================================
+ Coverage 68.27% 70.23% +1.95%
- Complexity 5262 6026 +764
============================================
Files 2049 2049
Lines 111033 111134 +101
Branches 16891 16917 +26
============================================
+ Hits 75810 78050 +2240
+ Misses 29816 27617 -2199
- Partials 5407 5467 +60
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 194 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
When can the else clause be hit?
There was a problem hiding this comment.
There are a lot of extensions of SqlNode. I have tried to cover the ones which are hit through most common query patterns. In cases of SqlDelete, SqlCreate we would hit else clause. In those cases, we would return an empty list for now.
...t-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
Outdated
Show resolved
Hide resolved
...t-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
Outdated
Show resolved
Hide resolved
...t-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
Outdated
Show resolved
Hide resolved
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
Outdated
Show resolved
Hide resolved
...t-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
Outdated
Show resolved
Hide resolved
tibrewalpratik17
force-pushed
the
allow_controller_multistage
branch
3 times, most recently
from
9ecfc05 to
1083a1e
Compare
walterddr
left a comment
walterddr
left a comment
There was a problem hiding this comment.
looks good to me mostly. can we add a integration test just to make sure this is covered? you can extend the MultistageIntegrationTest and add additional tenant for verification
tibrewalpratik17
force-pushed
the
allow_controller_multistage
branch
from
7eea3a0 to
d705e89
Compare
tibrewalpratik17
force-pushed
the
allow_controller_multistage
branch
from
d705e89 to
6ad8b7a
Compare
pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java
Outdated
Show resolved
Hide resolved
...t-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
Outdated
Show resolved
Hide resolved
walterddr
left a comment
walterddr
left a comment
There was a problem hiding this comment.
looks good to me overall. have some follow up comments .please kindly take a look
...t-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
Outdated
Show resolved
Hide resolved
tibrewalpratik17
force-pushed
the
allow_controller_multistage
branch
from
77d0487 to
142b2c4
Compare
|
looks like this broke explain |
|
Yeah this had. @tibrewalpratik17 had merged this which should have fixed it: #10505 Are explain queries still failing? |
5 participants
At present, via Controller UI we can only execute multistage queries on DefaultTenant. This patch checks if the tables queried are having atleast one common tenant and allows queries to be executed.