You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tibrewalpratik17 (via GitHub)" <gi...@apache.org> on 2023/06/21 13:23:06 UTC
[GitHub] [pinot] tibrewalpratik17 opened a new pull request, #10955: Update pinot client to not use Calcite extractTableNames
tibrewalpratik17 opened a new pull request, #10955:
URL: https://github.com/apache/pinot/pull/10955
As discussed in #10873 , removing pinot-client using `extractTableNamesFromNode` which got introduced in #10692.
Now in case of multistage queries, it will randomly select a broker.
cc @walterddr @abhioncbr
--
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] xiangfu0 commented on pull request #10955: Update pinot client to not use Calcite extractTableNames
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #10955:
URL: https://github.com/apache/pinot/pull/10955#issuecomment-1626212461
Seeing an exception thrown due to comma join not supported.
See more: https://github.com/apache/pinot/issues/11065
--
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 #10955: Update pinot client to not use Calcite extractTableNames
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #10955:
URL: https://github.com/apache/pinot/pull/10955
--
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 #10955: Update pinot client to not use Calcite extractTableNames
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10955:
URL: https://github.com/apache/pinot/pull/10955#discussion_r1237852502
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/Connection.java:
##########
@@ -190,8 +192,10 @@ public Future<ResultSetGroup> executeAsync(@Nullable String tableName, String qu
private static String[] resolveTableName(String query) {
try {
SqlNodeAndOptions sqlNodeAndOptions = CalciteSqlParser.compileToSqlNodeAndOptions(query);
- List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());
- return tableNames.toArray(new String[0]);
+ String tableName = (sqlNodeAndOptions.getSqlNode() != null
Review Comment:
sqlNodeAndOptions.getSqlNode() will never be null, this check is not needed
--
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] tibrewalpratik17 commented on a diff in pull request #10955: Update pinot client to not use Calcite extractTableNames
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #10955:
URL: https://github.com/apache/pinot/pull/10955#discussion_r1238255857
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/Connection.java:
##########
@@ -190,8 +192,10 @@ public Future<ResultSetGroup> executeAsync(@Nullable String tableName, String qu
private static String[] resolveTableName(String query) {
try {
SqlNodeAndOptions sqlNodeAndOptions = CalciteSqlParser.compileToSqlNodeAndOptions(query);
- List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());
- return tableNames.toArray(new String[0]);
+ String tableName = (sqlNodeAndOptions.getSqlNode() != null
Review Comment:
@walterddr updated this!
On a side note, i wonder if we can remove the same logic from here as well - https://github.com/apache/pinot/blob/2b47dd68926dd39da05949fc204388797dfd14cb/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java#L232-L234
--
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 pull request #10955: Update pinot client to not use Calcite extractTableNames
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10955:
URL: https://github.com/apache/pinot/pull/10955#issuecomment-1627529821
^ looks like it is not related to v2 extract table but with v1 yes?
--
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 #10955: Update pinot client to not use Calcite extractTableNames
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10955:
URL: https://github.com/apache/pinot/pull/10955#issuecomment-1600927203
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10955?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10955](https://app.codecov.io/gh/apache/pinot/pull/10955?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (fee0d82) into [master](https://app.codecov.io/gh/apache/pinot/commit/73830ae3ce94428b7e9991d1bd160bd62a14b7c1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (73830ae) will **not change** coverage.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10955 +/- ##
=======================================
Coverage 0.11% 0.11%
=======================================
Files 2188 2188
Lines 117749 117751 +2
Branches 17796 17797 +1
=======================================
Hits 137 137
- Misses 117592 117594 +2
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `0.00% <0.00%> (ø)` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `0.00% <0.00%> (ø)` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `0.11% <0.00%> (ø)` | |
| unittests2temurin17 | `0.11% <0.00%> (ø)` | |
| unittests2temurin20 | `0.11% <0.00%> (ø)` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10955?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../main/java/org/apache/pinot/client/Connection.java](https://app.codecov.io/gh/apache/pinot/pull/10955?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `0.00% <0.00%> (ø)` | |
: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=apache)
--
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 #10955: Update pinot client to not use Calcite extractTableNames
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10955:
URL: https://github.com/apache/pinot/pull/10955#discussion_r1237853380
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/Connection.java:
##########
@@ -190,8 +192,10 @@ public Future<ResultSetGroup> executeAsync(@Nullable String tableName, String qu
private static String[] resolveTableName(String query) {
try {
SqlNodeAndOptions sqlNodeAndOptions = CalciteSqlParser.compileToSqlNodeAndOptions(query);
- List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());
- return tableNames.toArray(new String[0]);
+ String tableName = (sqlNodeAndOptions.getSqlNode() != null
+ ? RequestUtils.getTableName(CalciteSqlParser.compileSqlNodeToPinotQuery(sqlNodeAndOptions.getSqlNode()))
+ : CalciteSqlCompiler.compileToBrokerRequest(query).getQuerySource().getTableName());
+ return new String[]{tableName};
} catch (Exception e) {
LOGGER.error("Cannot parse table name from query: {}", query, e);
Review Comment:
nit we can add additional comments
```suggestion
LOGGER.error("Cannot parse table name from query: {}. Fallback to broker selector default.", query, e);
```
--
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] tibrewalpratik17 commented on a diff in pull request #10955: Update pinot client to not use Calcite extractTableNames
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #10955:
URL: https://github.com/apache/pinot/pull/10955#discussion_r1238255857
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/Connection.java:
##########
@@ -190,8 +192,10 @@ public Future<ResultSetGroup> executeAsync(@Nullable String tableName, String qu
private static String[] resolveTableName(String query) {
try {
SqlNodeAndOptions sqlNodeAndOptions = CalciteSqlParser.compileToSqlNodeAndOptions(query);
- List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());
- return tableNames.toArray(new String[0]);
+ String tableName = (sqlNodeAndOptions.getSqlNode() != null
Review Comment:
@walterddr updated this!
On a side note, i wonder if we can remove the same logic from here as well - https://github.com/apache/pinot/blob/2b47dd68926dd39da05949fc204388797dfd14cb/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java#L232
--
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