You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "aadilkhalifa (via GitHub)" <gi...@apache.org> on 2024/03/02 07:14:55 UTC
[PR] DRAFT: Adding SHOW TABLES/DESCRIBE table support [pinot]
aadilkhalifa opened a new pull request, #12537:
URL: https://github.com/apache/pinot/pull/12537
PR to add support for "SHOW TABLES" and "DESCRIBE table" queries.
Current status:
- [x] SHOW TABLES
- [ ] DESCRIBE table
--
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
Re: [PR] DRAFT: Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1519153285
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -150,10 +153,12 @@ public abstract class BaseBrokerRequestHandler implements BrokerRequestHandler {
private final boolean _enableQueryLimitOverride;
private final boolean _enableDistinctCountBitmapOverride;
private final Map<Long, QueryServers> _queriesById;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
public BaseBrokerRequestHandler(PinotConfiguration config, String brokerId, BrokerRoutingManager routingManager,
AccessControlFactory accessControlFactory, QueryQuotaManager queryQuotaManager, TableCache tableCache,
- BrokerMetrics brokerMetrics, BrokerQueryEventListener brokerQueryEventListener) {
+ BrokerMetrics brokerMetrics, BrokerQueryEventListener brokerQueryEventListener,
Review Comment:
@xiangfu0 I've replaced with TableCache now.
--
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
Re: [PR] DRAFT: Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1511663907
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -150,10 +153,12 @@ public abstract class BaseBrokerRequestHandler implements BrokerRequestHandler {
private final boolean _enableQueryLimitOverride;
private final boolean _enableDistinctCountBitmapOverride;
private final Map<Long, QueryServers> _queriesById;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
public BaseBrokerRequestHandler(PinotConfiguration config, String brokerId, BrokerRoutingManager routingManager,
AccessControlFactory accessControlFactory, QueryQuotaManager queryQuotaManager, TableCache tableCache,
- BrokerMetrics brokerMetrics, BrokerQueryEventListener brokerQueryEventListener) {
+ BrokerMetrics brokerMetrics, BrokerQueryEventListener brokerQueryEventListener,
Review Comment:
See if you can reuse TableCache for this, no need to set `propertyStore` here.
--
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
Re: [PR] DRAFT: Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1511836215
##########
pinot-common/src/main/java/org/apache/pinot/common/request/PinotQuery.java:
##########
@@ -41,6 +41,8 @@ public class PinotQuery implements org.apache.thrift.TBase<PinotQuery, PinotQuer
private static final org.apache.thrift.protocol.TField DEBUG_OPTIONS_FIELD_DESC = new org.apache.thrift.protocol.TField("debugOptions", org.apache.thrift.protocol.TType.MAP, (short)10);
private static final org.apache.thrift.protocol.TField QUERY_OPTIONS_FIELD_DESC = new org.apache.thrift.protocol.TField("queryOptions", org.apache.thrift.protocol.TType.MAP, (short)11);
private static final org.apache.thrift.protocol.TField EXPLAIN_FIELD_DESC = new org.apache.thrift.protocol.TField("explain", org.apache.thrift.protocol.TType.BOOL, (short)12);
+ private static final org.apache.thrift.protocol.TField SHOW_TABLES_FIELD_DESC = new org.apache.thrift.protocol.TField("showTables", org.apache.thrift.protocol.TType.BOOL, (short)12);
Review Comment:
Is this file generated by thrift?
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #12537:
URL: https://github.com/apache/pinot/pull/12537#issuecomment-4686034322
Thanks for the contribution. Closing this out because the SQL DDL support landed recently in #18241, including SHOW TABLES and table inspection via SHOW CREATE TABLE. Please use/follow up on #18241 for any remaining gaps.
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1676834213
##########
pinot-common/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -109,3 +109,14 @@ SqlNode SqlPhysicalExplain() :
nDynamicParams);
}
}
+SqlNode SqlShowTables() :
+{
+ SqlParserPos pos;
+}
+{
+ <SHOW> { pos = getPos(); }
Review Comment:
Updated 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
Re: [PR] DRAFT: Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1582876981
##########
pinot-common/src/thrift/query.thrift:
##########
@@ -32,6 +32,8 @@ struct PinotQuery {
11: optional map<string, string> queryOptions;
12: optional bool explain;
13: optional map<Expression, Expression> expressionOverrideHints;
+ 14: optional bool showTables;
Review Comment:
I've removed it from `PinotQuery` now
--
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
Re: [PR] DRAFT: Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1519153810
##########
pinot-common/src/main/java/org/apache/pinot/common/request/PinotQuery.java:
##########
@@ -41,6 +41,8 @@ public class PinotQuery implements org.apache.thrift.TBase<PinotQuery, PinotQuer
private static final org.apache.thrift.protocol.TField DEBUG_OPTIONS_FIELD_DESC = new org.apache.thrift.protocol.TField("debugOptions", org.apache.thrift.protocol.TType.MAP, (short)10);
private static final org.apache.thrift.protocol.TField QUERY_OPTIONS_FIELD_DESC = new org.apache.thrift.protocol.TField("queryOptions", org.apache.thrift.protocol.TType.MAP, (short)11);
private static final org.apache.thrift.protocol.TField EXPLAIN_FIELD_DESC = new org.apache.thrift.protocol.TField("explain", org.apache.thrift.protocol.TType.BOOL, (short)12);
+ private static final org.apache.thrift.protocol.TField SHOW_TABLES_FIELD_DESC = new org.apache.thrift.protocol.TField("showTables", org.apache.thrift.protocol.TType.BOOL, (short)12);
Review Comment:
@xiangfu0 I've used thrift generated file now.
--
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
Re: [PR] DRAFT: Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1569749650
##########
pinot-common/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -109,3 +109,14 @@ SqlNode SqlPhysicalExplain() :
nDynamicParams);
}
}
+SqlNode SqlShowTables() :
+{
+ SqlParserPos pos;
+}
+{
+ <SHOW> { pos = getPos(); }
Review Comment:
Do we need to modify the parser? Is there a standard SQL way of showing tables?
##########
pinot-common/src/thrift/query.thrift:
##########
@@ -32,6 +32,8 @@ struct PinotQuery {
11: optional map<string, string> queryOptions;
12: optional bool explain;
13: optional map<Expression, Expression> expressionOverrideHints;
+ 14: optional bool showTables;
Review Comment:
Do we need to add it into `PinotQuery`? I don't think we need to send the request to server. Ideally we can get this info from the parsed `PlanNode` without constructing the `PinotQuery`
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1719414769
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -269,10 +271,22 @@ private String getQueryResponse(String query, @Nullable SqlNode sqlNode, String
return QueryException.getException(QueryException.QUERY_VALIDATION_ERROR, e).toString();
}
try {
- String inputTableName =
- sqlNode != null ? RequestUtils.getTableNames(CalciteSqlParser.compileSqlNodeToPinotQuery(sqlNode)).iterator()
- .next() : CalciteSqlCompiler.compileToBrokerRequest(query).getQuerySource().getTableName();
- tableName = _pinotHelixResourceManager.getActualTableName(inputTableName, database);
+ List<String> inputTableNames = new ArrayList<>();
+ if (sqlNode != null) {
+ if (sqlNode instanceof SqlShowTables) {
+ inputTableNames.addAll(_pinotHelixResourceManager.getAllTables(database));
Review Comment:
We don't need query access to all tables in order to run SHOW TABLES. Instead, we can take the same action as `PinotTableRestletResource.listTables()` which also list all tables.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -269,10 +271,22 @@ private String getQueryResponse(String query, @Nullable SqlNode sqlNode, String
return QueryException.getException(QueryException.QUERY_VALIDATION_ERROR, e).toString();
}
try {
- String inputTableName =
- sqlNode != null ? RequestUtils.getTableNames(CalciteSqlParser.compileSqlNodeToPinotQuery(sqlNode)).iterator()
- .next() : CalciteSqlCompiler.compileToBrokerRequest(query).getQuerySource().getTableName();
- tableName = _pinotHelixResourceManager.getActualTableName(inputTableName, database);
+ List<String> inputTableNames = new ArrayList<>();
+ if (sqlNode != null) {
+ if (sqlNode instanceof SqlShowTables) {
+ inputTableNames.addAll(_pinotHelixResourceManager.getAllTables(database));
+ } else if (sqlNode instanceof SqlDescribeTable) {
+ inputTableNames.add(((SqlDescribeTable) sqlNode).getTable().toString());
Review Comment:
Similarly, we should check for access with same action as `PinotTableSchema.getTableSchema()`
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -1857,4 +1868,65 @@ private static class QueryServers {
}
}
}
+
+ private BrokerResponseNative processShowTablesQuery(SqlNodeAndOptions sqlNodeAndOptions, HttpHeaders httpHeaders,
+ long compilationStartTimeNs, RequestContext requestContext) {
+ Map<String, String> queryOptions = sqlNodeAndOptions.getOptions();
Review Comment:
Currently the access control is only added on the controller side (`PinotQueryResource`). We should also perform access control on broker (in this class)
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1676834537
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -1558,6 +1571,60 @@ private void computeResultsForLiteral(Literal literal, List<String> columnNames,
}
}
+ private BrokerResponseNative processShowTablesQuery(long compilationStartTimeNs,
+ RequestContext requestContext) {
+ BrokerResponseNative brokerResponse = new BrokerResponseNative();
+ String[] columnNames = {"Tables"};
+ DataSchema.ColumnDataType[] columnTypes = {DataSchema.ColumnDataType.STRING};
+
+
+ DataSchema dataSchema = new DataSchema(columnNames, columnTypes);
+ List<Object[]> rows = new ArrayList<>();
+ _tableCache.getTableConfigs().stream().map(TableConfig::getTableName).sorted().forEach(a -> {
Review Comment:
I've added a filter to return only the tables within the database
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1676834786
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -261,9 +263,19 @@ private String getQueryResponse(String query, @Nullable SqlNode sqlNode, String
return QueryException.getException(QueryException.QUERY_VALIDATION_ERROR, e).toString();
}
try {
- String inputTableName =
- sqlNode != null ? RequestUtils.getTableNames(CalciteSqlParser.compileSqlNodeToPinotQuery(sqlNode)).iterator()
- .next() : CalciteSqlCompiler.compileToBrokerRequest(query).getQuerySource().getTableName();
+ String inputTableName;
+ if (sqlNode != null) {
+ if (sqlNode instanceof SqlShowTables) {
+ inputTableName = "*";
Review Comment:
I've removed this and changed the logic now
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1895325216
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -269,10 +271,22 @@ private String getQueryResponse(String query, @Nullable SqlNode sqlNode, String
return QueryException.getException(QueryException.QUERY_VALIDATION_ERROR, e).toString();
}
try {
- String inputTableName =
- sqlNode != null ? RequestUtils.getTableNames(CalciteSqlParser.compileSqlNodeToPinotQuery(sqlNode)).iterator()
- .next() : CalciteSqlCompiler.compileToBrokerRequest(query).getQuerySource().getTableName();
- tableName = _pinotHelixResourceManager.getActualTableName(inputTableName, database);
+ List<String> inputTableNames = new ArrayList<>();
+ if (sqlNode != null) {
+ if (sqlNode instanceof SqlShowTables) {
+ inputTableNames.addAll(_pinotHelixResourceManager.getAllTables(database));
Review Comment:
I've made this change. Can you please have a look?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -269,10 +271,22 @@ private String getQueryResponse(String query, @Nullable SqlNode sqlNode, String
return QueryException.getException(QueryException.QUERY_VALIDATION_ERROR, e).toString();
}
try {
- String inputTableName =
- sqlNode != null ? RequestUtils.getTableNames(CalciteSqlParser.compileSqlNodeToPinotQuery(sqlNode)).iterator()
- .next() : CalciteSqlCompiler.compileToBrokerRequest(query).getQuerySource().getTableName();
- tableName = _pinotHelixResourceManager.getActualTableName(inputTableName, database);
+ List<String> inputTableNames = new ArrayList<>();
+ if (sqlNode != null) {
+ if (sqlNode instanceof SqlShowTables) {
+ inputTableNames.addAll(_pinotHelixResourceManager.getAllTables(database));
+ } else if (sqlNode instanceof SqlDescribeTable) {
+ inputTableNames.add(((SqlDescribeTable) sqlNode).getTable().toString());
Review Comment:
I've made this change.
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1895325413
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -1857,4 +1868,65 @@ private static class QueryServers {
}
}
}
+
+ private BrokerResponseNative processShowTablesQuery(SqlNodeAndOptions sqlNodeAndOptions, HttpHeaders httpHeaders,
+ long compilationStartTimeNs, RequestContext requestContext) {
+ Map<String, String> queryOptions = sqlNodeAndOptions.getOptions();
Review Comment:
I've made this change.
--
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
Re: [PR] DRAFT: Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1582877639
##########
pinot-common/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -109,3 +109,14 @@ SqlNode SqlPhysicalExplain() :
nDynamicParams);
}
}
+SqlNode SqlShowTables() :
+{
+ SqlParserPos pos;
+}
+{
+ <SHOW> { pos = getPos(); }
Review Comment:
I don't think `SHOW` keyword is part of the parser, so I had to add it.
--
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
Re: [PR] DRAFT: Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12537:
URL: https://github.com/apache/pinot/pull/12537#issuecomment-1974751774
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12537?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `28 lines` in your changes are missing coverage. Please review.
> Project coverage is 0.00%. Comparing base [(`640ebe5`)](https://app.codecov.io/gh/apache/pinot/commit/640ebe52cf19922530b67f354126d007e3b650a2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`49119d8`)](https://app.codecov.io/gh/apache/pinot/pull/12537?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 142 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...t/controller/api/resources/PinotQueryResource.java](https://app.codecov.io/gh/apache/pinot/pull/12537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90UXVlcnlSZXNvdXJjZS5qYXZh) | 0.00% | [18 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...apache/pinot/sql/parsers/parser/SqlShowTables.java](https://app.codecov.io/gh/apache/pinot/pull/12537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9wYXJzZXIvU3FsU2hvd1RhYmxlcy5qYXZh) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://app.codecov.io/gh/apache/pinot/pull/12537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12537 +/- ##
=============================================
- Coverage 61.58% 0.00% -61.59%
=============================================
Files 2415 2375 -40
Lines 131502 129828 -1674
Branches 20310 20137 -173
=============================================
- Hits 80988 0 -80988
- Misses 44587 129828 +85241
+ Partials 5927 0 -5927
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.48%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.55%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.59%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12537?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?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
Re: [PR] DRAFT: Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1510984829
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -243,10 +246,17 @@ private String getQueryResponse(String query, @Nullable SqlNode sqlNode, String
// Get resource table name.
String tableName;
try {
- String inputTableName =
- sqlNode != null ? RequestUtils.getTableNames(CalciteSqlParser.compileSqlNodeToPinotQuery(sqlNode)).iterator()
- .next() : CalciteSqlCompiler.compileToBrokerRequest(query).getQuerySource().getTableName();
- tableName = _pinotHelixResourceManager.getActualTableName(inputTableName);
+ if (sqlNode != null) {
+ PinotQuery pinotQuery = CalciteSqlParser.compileSqlNodeToPinotQuery(sqlNode);
+ if (pinotQuery.isShowTables()) {
+ return showTablesResponse();
+ }
+ String inputTableName = RequestUtils.getTableNames(pinotQuery).iterator().next();
+ tableName = _pinotHelixResourceManager.getActualTableName(inputTableName);
Review Comment:
@xiangfu0 I've moved code to broker now. Can you have a look?
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1676834311
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -421,6 +423,9 @@ public static PinotQuery compileSqlNodeToPinotQuery(SqlNode sqlNode) {
sqlNode = ((SqlExplain) sqlNode).getExplicandum();
pinotQuery.setExplain(true);
}
+ if (sqlNode instanceof SqlShowTables || sqlNode instanceof SqlDescribeTable) {
+ return pinotQuery;
Review Comment:
Yes, we don't need the further processing steps, so returning here
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1597256279
##########
pinot-common/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -109,3 +109,14 @@ SqlNode SqlPhysicalExplain() :
nDynamicParams);
}
}
+SqlNode SqlShowTables() :
Review Comment:
(format) Add an empty line in front of the function
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/parser/SqlShowTables.java:
##########
@@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.sql.parsers.parser;
+
+import java.util.List;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlSpecialOperator;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.sql.parser.SqlParserPos;
+
+public class SqlShowTables extends SqlCall {
+
+ private static final SqlSpecialOperator OPERATOR = new SqlSpecialOperator("UDF", SqlKind.OTHER);
+
+ protected SqlShowTables(SqlParserPos pos) {
+ super(pos);
+ }
+
+ @Override public SqlOperator getOperator() {
Review Comment:
(format) Reformat the changes, same for other places
##########
pinot-common/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -109,3 +109,14 @@ SqlNode SqlPhysicalExplain() :
nDynamicParams);
}
}
+SqlNode SqlShowTables() :
+{
+ SqlParserPos pos;
+}
+{
+ <SHOW> { pos = getPos(); }
Review Comment:
Here is the example of `SqlShowTables` in Calcite, should we keep `<SHOW>` and `<TABLES>` together?
```
Example of SqlShowTables() implementation:
SqlNode SqlShowTables()
{
...local variables...
}
{
<SHOW> <TABLES>
...
{
return SqlShowTables(...)
}
}
```
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -421,6 +423,9 @@ public static PinotQuery compileSqlNodeToPinotQuery(SqlNode sqlNode) {
sqlNode = ((SqlExplain) sqlNode).getExplicandum();
pinotQuery.setExplain(true);
}
+ if (sqlNode instanceof SqlShowTables || sqlNode instanceof SqlDescribeTable) {
+ return pinotQuery;
Review Comment:
Does it work if we directly return empty query here?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -261,9 +263,19 @@ private String getQueryResponse(String query, @Nullable SqlNode sqlNode, String
return QueryException.getException(QueryException.QUERY_VALIDATION_ERROR, e).toString();
}
try {
- String inputTableName =
- sqlNode != null ? RequestUtils.getTableNames(CalciteSqlParser.compileSqlNodeToPinotQuery(sqlNode)).iterator()
- .next() : CalciteSqlCompiler.compileToBrokerRequest(query).getQuerySource().getTableName();
+ String inputTableName;
+ if (sqlNode != null) {
+ if (sqlNode instanceof SqlShowTables) {
+ inputTableName = "*";
Review Comment:
Trying to follow how does this work. What does `_pinotHelixResourceManager.getActualTableName("*", database)` return?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -1558,6 +1571,60 @@ private void computeResultsForLiteral(Literal literal, List<String> columnNames,
}
}
+ private BrokerResponseNative processShowTablesQuery(long compilationStartTimeNs,
+ RequestContext requestContext) {
+ BrokerResponseNative brokerResponse = new BrokerResponseNative();
+ String[] columnNames = {"Tables"};
+ DataSchema.ColumnDataType[] columnTypes = {DataSchema.ColumnDataType.STRING};
+
+
+ DataSchema dataSchema = new DataSchema(columnNames, columnTypes);
+ List<Object[]> rows = new ArrayList<>();
+ _tableCache.getTableConfigs().stream().map(TableConfig::getTableName).sorted().forEach(a -> {
Review Comment:
We cannot return all tables here. It needs to filter on the database, and only return the tables within the database
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -1558,6 +1571,60 @@ private void computeResultsForLiteral(Literal literal, List<String> columnNames,
}
}
+ private BrokerResponseNative processShowTablesQuery(long compilationStartTimeNs,
+ RequestContext requestContext) {
+ BrokerResponseNative brokerResponse = new BrokerResponseNative();
+ String[] columnNames = {"Tables"};
+ DataSchema.ColumnDataType[] columnTypes = {DataSchema.ColumnDataType.STRING};
+
Review Comment:
(format) Multiple empty lines
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -1558,6 +1571,60 @@ private void computeResultsForLiteral(Literal literal, List<String> columnNames,
}
}
+ private BrokerResponseNative processShowTablesQuery(long compilationStartTimeNs,
+ RequestContext requestContext) {
+ BrokerResponseNative brokerResponse = new BrokerResponseNative();
+ String[] columnNames = {"Tables"};
+ DataSchema.ColumnDataType[] columnTypes = {DataSchema.ColumnDataType.STRING};
+
+
+ DataSchema dataSchema = new DataSchema(columnNames, columnTypes);
+ List<Object[]> rows = new ArrayList<>();
+ _tableCache.getTableConfigs().stream().map(TableConfig::getTableName).sorted().forEach(a -> {
+ a = a.replace("_OFFLINE", "").replace("_REALTIME", "");
+ rows.add(new String[] {a});
+ });
+ ResultTable resultTable = new ResultTable(dataSchema, rows);
+ brokerResponse.setResultTable(resultTable);
+
+ long totalTimeMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - compilationStartTimeNs);
+ brokerResponse.setTimeUsedMs(totalTimeMs);
+ requestContext.setQueryProcessingTime(totalTimeMs);
+ augmentStatistics(requestContext, brokerResponse);
+ return brokerResponse;
+ }
+
+ private BrokerResponseNative processDescribeTableQuery(String tableName, long compilationStartTimeNs,
Review Comment:
We need to handle `database` and access control here
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1676834328
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/parser/SqlShowTables.java:
##########
@@ -0,0 +1,52 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.sql.parsers.parser;
+
+import java.util.List;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlSpecialOperator;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.sql.parser.SqlParserPos;
+
+public class SqlShowTables extends SqlCall {
+
+ private static final SqlSpecialOperator OPERATOR = new SqlSpecialOperator("UDF", SqlKind.OTHER);
+
+ protected SqlShowTables(SqlParserPos pos) {
+ super(pos);
+ }
+
+ @Override public SqlOperator getOperator() {
Review Comment:
Done
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -1558,6 +1571,60 @@ private void computeResultsForLiteral(Literal literal, List<String> columnNames,
}
}
+ private BrokerResponseNative processShowTablesQuery(long compilationStartTimeNs,
+ RequestContext requestContext) {
+ BrokerResponseNative brokerResponse = new BrokerResponseNative();
+ String[] columnNames = {"Tables"};
+ DataSchema.ColumnDataType[] columnTypes = {DataSchema.ColumnDataType.STRING};
+
Review Comment:
Done
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1676834608
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -1558,6 +1571,60 @@ private void computeResultsForLiteral(Literal literal, List<String> columnNames,
}
}
+ private BrokerResponseNative processShowTablesQuery(long compilationStartTimeNs,
+ RequestContext requestContext) {
+ BrokerResponseNative brokerResponse = new BrokerResponseNative();
+ String[] columnNames = {"Tables"};
+ DataSchema.ColumnDataType[] columnTypes = {DataSchema.ColumnDataType.STRING};
+
+
+ DataSchema dataSchema = new DataSchema(columnNames, columnTypes);
+ List<Object[]> rows = new ArrayList<>();
+ _tableCache.getTableConfigs().stream().map(TableConfig::getTableName).sorted().forEach(a -> {
+ a = a.replace("_OFFLINE", "").replace("_REALTIME", "");
+ rows.add(new String[] {a});
+ });
+ ResultTable resultTable = new ResultTable(dataSchema, rows);
+ brokerResponse.setResultTable(resultTable);
+
+ long totalTimeMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - compilationStartTimeNs);
+ brokerResponse.setTimeUsedMs(totalTimeMs);
+ requestContext.setQueryProcessingTime(totalTimeMs);
+ augmentStatistics(requestContext, brokerResponse);
+ return brokerResponse;
+ }
+
+ private BrokerResponseNative processDescribeTableQuery(String tableName, long compilationStartTimeNs,
Review Comment:
I've added check for database tables access. Any other access control required?
--
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
Re: [PR] DRAFT: Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1511663907
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -150,10 +153,12 @@ public abstract class BaseBrokerRequestHandler implements BrokerRequestHandler {
private final boolean _enableQueryLimitOverride;
private final boolean _enableDistinctCountBitmapOverride;
private final Map<Long, QueryServers> _queriesById;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
public BaseBrokerRequestHandler(PinotConfiguration config, String brokerId, BrokerRoutingManager routingManager,
AccessControlFactory accessControlFactory, QueryQuotaManager queryQuotaManager, TableCache tableCache,
- BrokerMetrics brokerMetrics, BrokerQueryEventListener brokerQueryEventListener) {
+ BrokerMetrics brokerMetrics, BrokerQueryEventListener brokerQueryEventListener,
Review Comment:
See if you can reuse TableCache for 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
Re: [PR] DRAFT: Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1509943974
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -243,10 +246,17 @@ private String getQueryResponse(String query, @Nullable SqlNode sqlNode, String
// Get resource table name.
String tableName;
try {
- String inputTableName =
- sqlNode != null ? RequestUtils.getTableNames(CalciteSqlParser.compileSqlNodeToPinotQuery(sqlNode)).iterator()
- .next() : CalciteSqlCompiler.compileToBrokerRequest(query).getQuerySource().getTableName();
- tableName = _pinotHelixResourceManager.getActualTableName(inputTableName);
+ if (sqlNode != null) {
+ PinotQuery pinotQuery = CalciteSqlParser.compileSqlNodeToPinotQuery(sqlNode);
+ if (pinotQuery.isShowTables()) {
+ return showTablesResponse();
+ }
+ String inputTableName = RequestUtils.getTableNames(pinotQuery).iterator().next();
+ tableName = _pinotHelixResourceManager.getActualTableName(inputTableName);
Review Comment:
this is for pinot controller only, what if the query is sent to pinot broker?
I feel you can call getTables API for response then wrap it as query response.
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "aadilkhalifa (via GitHub)" <gi...@apache.org>.
aadilkhalifa commented on code in PR #12537:
URL: https://github.com/apache/pinot/pull/12537#discussion_r1676834190
##########
pinot-common/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -109,3 +109,14 @@ SqlNode SqlPhysicalExplain() :
nDynamicParams);
}
}
+SqlNode SqlShowTables() :
Review Comment:
Done
--
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
Re: [PR] Adding SHOW TABLES/DESCRIBE table support [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 closed pull request #12537: Adding SHOW TABLES/DESCRIBE table support
URL: https://github.com/apache/pinot/pull/12537
--
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