You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "abhioncbr (via GitHub)" <gi...@apache.org> on 2023/05/08 01:33:27 UTC
[GitHub] [pinot] abhioncbr opened a new pull request, #10739: 10736: Fix for self join table names extraction in multistage
abhioncbr opened a new pull request, #10739:
URL: https://github.com/apache/pinot/pull/10739
- Fix for extracting the table names in case of Self Join queries.
- Added test cases for Self Join queries.
As per the [issue](https://github.com/apache/pinot/issues/10736)
cc: @walterddr @ankitsultana @xiangfu0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #10739: 10736: Fix for self join table names extraction in multistage
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10739:
URL: https://github.com/apache/pinot/pull/10739#issuecomment-1537660878
I am not sure, but `integration test set 2` failures don't seem to be related to this issue.
```
Error: Failures:
Error: UrlAuthRealtimeIntegrationTest.setUp:70->ClusterTest.startServer:201->ClusterTest.startServers:209->ClusterTest.startOneServer:217 » Runtime
Error: UrlAuthRealtimeIntegrationTest.tearDown:89->dropRealtimeTable:185->ControllerTest.sendDeleteRequest:831 » IO
```
Also, failed tests are working fine in my local with the current changes. Anyone please re-trigger the execution of `integration test set 2`? Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr merged pull request #10739: [multistage] 10736: Fix for self join table names extraction in multistage
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #10739:
URL: https://github.com/apache/pinot/pull/10739
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on a diff in pull request #10739: [multistage] 10736: Fix for self join table names extraction in multistage
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10739:
URL: https://github.com/apache/pinot/pull/10739#discussion_r1189240441
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -3223,5 +3231,22 @@ public void testJoin() {
Assert.assertEquals(rightSubquery,
CalciteSqlParser.compileToPinotQuery("SELECT key, COUNT(*) AS b FROM T3 JOIN T4 GROUP BY key"));
Assert.assertEquals(join.getCondition(), CalciteSqlParser.compileToExpression("T1.key = T2.key"));
+
+ // test for self join queries
+ query = "SELECT T1.a FROM T1 JOIN(SELECT key FROM T1) as self ON T1.key=self.key";
Review Comment:
It is covering; I just added it to make sure `self` has no issues after the function call `extractTableNamesFromNode`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on a diff in pull request #10739: [multistage] 10736: Fix for self join table names extraction in multistage
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10739:
URL: https://github.com/apache/pinot/pull/10739#discussion_r1188695827
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -3223,5 +3231,22 @@ public void testJoin() {
Assert.assertEquals(rightSubquery,
CalciteSqlParser.compileToPinotQuery("SELECT key, COUNT(*) AS b FROM T3 JOIN T4 GROUP BY key"));
Assert.assertEquals(join.getCondition(), CalciteSqlParser.compileToExpression("T1.key = T2.key"));
+
+ // test for self join queries
+ query = "SELECT T1.a FROM T1 JOIN(SELECT key FROM T1) as self ON T1.key=self.key";
Review Comment:
As per the[ PG documentation ](https://www.postgresql.org/docs/current/sql-keywords-appendix.html), `self` is a non-reserved keyword. There is no difference between `self` and `AS T2`.
The issue was because of alias(`AS`) usage, not with `self`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10739: 10736: Fix for self join table names extraction in multistage
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10739:
URL: https://github.com/apache/pinot/pull/10739#issuecomment-1537640384
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10739?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10739](https://app.codecov.io/gh/apache/pinot/pull/10739?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (84d08c4) into [master](https://app.codecov.io/gh/apache/pinot/commit/7dcae9208e7f297fa559fd7e59be9ffeec09d14f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7dcae92) will **decrease** coverage by `56.50%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10739 +/- ##
=============================================
- Coverage 70.32% 13.83% -56.50%
+ Complexity 6462 439 -6023
=============================================
Files 2125 2071 -54
Lines 114368 111883 -2485
Branches 17259 16963 -296
=============================================
- Hits 80435 15475 -64960
- Misses 28325 95145 +66820
+ Partials 5608 1263 -4345
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.83% <0.00%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10739?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://app.codecov.io/gh/apache/pinot/pull/10739?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `0.00% <0.00%> (-84.65%)` | :arrow_down: |
... and [1677 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10739/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] abhioncbr commented on pull request #10739: [multistage] 10736: Fix for self join table names extraction in multistage
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10739:
URL: https://github.com/apache/pinot/pull/10739#issuecomment-1540765910
Can we re-trigger the failing integration test? I am unsure, but I see intermittent failures in the integration test. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10739: [multistage] 10736: Fix for self join table names extraction in multistage
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10739:
URL: https://github.com/apache/pinot/pull/10739#discussion_r1188067894
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -3223,5 +3231,22 @@ public void testJoin() {
Assert.assertEquals(rightSubquery,
CalciteSqlParser.compileToPinotQuery("SELECT key, COUNT(*) AS b FROM T3 JOIN T4 GROUP BY key"));
Assert.assertEquals(join.getCondition(), CalciteSqlParser.compileToExpression("T1.key = T2.key"));
+
+ // test for self join queries
+ query = "SELECT T1.a FROM T1 JOIN(SELECT key FROM T1) as self ON T1.key=self.key";
Review Comment:
is `self` a reserved keyword? what's the difference between this test and the one with ` AS T2` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10739: [multistage] 10736: Fix for self join table names extraction in multistage
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10739:
URL: https://github.com/apache/pinot/pull/10739#discussion_r1188068362
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -3138,6 +3138,14 @@ public void testExtractTableNamesFromNode() {
Assert.assertEquals(tableNames.get(1), "tbl2");
Assert.assertEquals(tableNames.get(2), "tbl3");
Assert.assertEquals(tableNames.get(3), "tbl4");
+
+ // test for self join queries
+ query = "SELECT tbl1.a FROM tbl1 JOIN(SELECT a FROM tbl1) as self ON tbl1.a=self.a ";
Review Comment:
technically the problem is not with self join but with the fact that JOIN implicit table with `AS alias` after the table definition.
can you also add a test with non self join. e.g. the same query but instead of both from tbl1, get one from tbl1 and one from tbl2?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10739: [multistage] 10736: Fix for self join table names extraction in multistage
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10739:
URL: https://github.com/apache/pinot/pull/10739#discussion_r1188919302
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -3223,5 +3231,22 @@ public void testJoin() {
Assert.assertEquals(rightSubquery,
CalciteSqlParser.compileToPinotQuery("SELECT key, COUNT(*) AS b FROM T3 JOIN T4 GROUP BY key"));
Assert.assertEquals(join.getCondition(), CalciteSqlParser.compileToExpression("T1.key = T2.key"));
+
+ // test for self join queries
+ query = "SELECT T1.a FROM T1 JOIN(SELECT key FROM T1) as self ON T1.key=self.key";
Review Comment:
^ in this case doesnt the AS T2 test already covered this ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org