Copilot commented on code in PR #17357:
URL: https://github.com/apache/pinot/pull/17357#discussion_r2616842180
##########
pinot-core/src/main/java/org/apache/pinot/core/query/executor/sql/SqlQueryExecutor.java:
##########
@@ -121,11 +143,207 @@ public BrokerResponse
executeDMLStatement(SqlNodeAndOptions sqlNodeAndOptions,
return result;
}
+ /**
+ * Execute metadata (SHOW/USE) statements.
+ */
+ public BrokerResponse executeMetadataStatement(SqlNodeAndOptions
sqlNodeAndOptions,
+ @Nullable Map<String, String> headers) {
+ BrokerResponseNative result = new BrokerResponseNative();
+ try {
+ ResultTable resultTable =
+ buildMetadataResult(sqlNodeAndOptions.getSqlNode(),
sqlNodeAndOptions.getOptions(), headers);
+ result.setResultTable(resultTable);
+ } catch (Exception e) {
+ result.addException(new
QueryProcessingException(QueryErrorCode.QUERY_EXECUTION, e.getMessage()));
+ }
+ return result;
+ }
+
private MinionClient getMinionClient() {
// NOTE: using null auth provider here as auth headers injected by caller
in "executeDMLStatement()"
if (_helixManager != null) {
return new MinionClient(getControllerBaseUrl(_helixManager), null);
}
return new MinionClient(_controllerUrl, null);
}
+
+ private ResultTable buildMetadataResult(SqlNode sqlNode, Map<String, String>
options,
+ @Nullable Map<String, String> headers)
+ throws Exception {
+ if (sqlNode instanceof SqlShowDatabases) {
+ return toSingleStringColumnResult("databaseName",
fetchDatabases(headers));
+ }
+ if (sqlNode instanceof SqlShowTables) {
+ SqlShowTables showTables = (SqlShowTables) sqlNode;
+ String database = resolveDatabase(showTables.getDatabaseName(), options,
headers);
+ List<String> tables = stripDatabasePrefix(fetchTables(database,
headers), database);
+ return toSingleStringColumnResult("tableName", tables);
+ }
+ if (sqlNode instanceof SqlShowSchemas) {
+ SqlShowSchemas showSchemas = (SqlShowSchemas) sqlNode;
+ String database = resolveDatabase(null, options, headers);
+ List<String> schemas = stripDatabasePrefix(fetchSchemas(database,
headers), database);
+ String likePattern = getLikePattern(showSchemas.getLikePattern());
+ schemas = applyLikeFilter(schemas, likePattern);
+ return toSingleStringColumnResult("schemaName", schemas);
+ }
+ throw new UnsupportedOperationException("Unsupported METADATA SqlKind - "
+ sqlNode.getKind());
+ }
+
+ private ResultTable toSingleStringColumnResult(String columnName,
List<String> values) {
+ DataSchema dataSchema = new DataSchema(new String[]{columnName}, new
ColumnDataType[]{ColumnDataType.STRING});
+ List<Object[]> rows = values.stream().map(value -> new
Object[]{value}).collect(Collectors.toList());
+ return new ResultTable(dataSchema, rows);
+ }
+
+ private String resolveDatabase(@Nullable SqlIdentifier explicitDatabase,
Map<String, String> options,
+ @Nullable Map<String, String> headers)
+ throws DatabaseConflictException {
+ String databaseFromSql = explicitDatabase != null ?
explicitDatabase.toString() : null;
+ String databaseFromOptions = options.get(CommonConstants.DATABASE);
+ String databaseFromHeaders = getHeaderValue(headers,
CommonConstants.DATABASE);
+
+ if (databaseFromSql != null) {
+ if (databaseFromOptions != null &&
!databaseFromSql.equalsIgnoreCase(databaseFromOptions)) {
+ throw new DatabaseConflictException(
+ "Database name '" + databaseFromSql + "' from statement does not
match database name '"
+ + databaseFromOptions + "' from query options and database
name '" + databaseFromHeaders
+ + "' from request header");
+ }
+ if (databaseFromHeaders != null &&
!databaseFromSql.equalsIgnoreCase(databaseFromHeaders)) {
+ throw new DatabaseConflictException(
+ "Database name '" + databaseFromSql + "' from statement does not
match database name '"
+ + databaseFromHeaders + "' from request header and database
name '" + databaseFromOptions
+ + "' from query options");
+ }
+ return databaseFromSql;
+ }
+
+ if (databaseFromOptions != null && databaseFromHeaders != null
+ && !databaseFromOptions.equalsIgnoreCase(databaseFromHeaders)) {
+ throw new DatabaseConflictException("Database name '" +
databaseFromHeaders + "' from request header does not "
+ + "match database name '" + databaseFromOptions + "' from query
options");
+ }
+
+ return databaseFromOptions != null ? databaseFromOptions
+ : databaseFromHeaders != null ? databaseFromHeaders :
CommonConstants.DEFAULT_DATABASE;
+ }
+
+ private List<String> stripDatabasePrefix(List<String> names, String
database) {
+ if (names.isEmpty()) {
+ return names;
+ }
+ return names.stream()
+ .map(name -> DatabaseUtils.isPartOfDatabase(name, database)
+ ? DatabaseUtils.removeDatabasePrefix(name, database)
+ : name)
+ .collect(Collectors.toList());
+ }
+
+ private List<String> applyLikeFilter(List<String> values, @Nullable String
likePattern) {
+ if (StringUtils.isEmpty(likePattern)) {
+ return values;
+ }
+ Pattern pattern = buildLikeRegex(likePattern);
+ return values.stream().filter(value ->
pattern.matcher(value).matches()).collect(Collectors.toList());
+ }
+
+ private Pattern buildLikeRegex(String likePattern) {
+ StringBuilder regex = new StringBuilder();
+ boolean escaped = false;
+ for (char c : likePattern.toCharArray()) {
+ if (escaped) {
+ regex.append(Pattern.quote(String.valueOf(c)));
+ escaped = false;
+ } else if (c == '\\') {
+ escaped = true;
+ } else if (c == '%') {
+ regex.append(".*");
+ } else if (c == '_') {
+ regex.append('.');
+ } else {
+ regex.append(Pattern.quote(String.valueOf(c)));
+ }
+ }
+ return Pattern.compile(regex.toString(), Pattern.CASE_INSENSITIVE);
+ }
+
+ private String getLikePattern(@Nullable SqlLiteral likeLiteral) {
+ return likeLiteral == null ? null : likeLiteral.toValue();
+ }
+
+ private List<String> fetchDatabases(@Nullable Map<String, String> headers)
+ throws IOException {
+ String response = sendGetRequest("/databases", headers);
+ JsonNode jsonNode = JsonUtils.stringToJsonNode(response);
+ List<String> databases = new ArrayList<>();
+ jsonNode.forEach(node -> databases.add(node.asText()));
+ return databases;
+ }
+
+ private List<String> fetchTables(String database, @Nullable Map<String,
String> headers)
+ throws IOException {
+ Map<String, String> requestHeaders = new HashMap<>();
+ if (headers != null) {
+ requestHeaders.putAll(headers);
+ }
+ requestHeaders.put(CommonConstants.DATABASE, database);
+ String response = sendGetRequest("/tables", requestHeaders);
+ JsonNode jsonNode = JsonUtils.stringToJsonNode(response);
+ JsonNode tablesNode = jsonNode.has("tables") ? jsonNode.get("tables") :
jsonNode;
+ List<String> tables = new ArrayList<>();
+ tablesNode.forEach(node -> tables.add(node.asText()));
+ return tables;
+ }
+
+ private List<String> fetchSchemas(String database, @Nullable Map<String,
String> headers)
+ throws IOException {
+ Map<String, String> requestHeaders = new HashMap<>();
+ if (headers != null) {
+ requestHeaders.putAll(headers);
+ }
+ requestHeaders.put(CommonConstants.DATABASE, database);
+ String response = sendGetRequest("/schemas", requestHeaders);
+ JsonNode jsonNode = JsonUtils.stringToJsonNode(response);
+ List<String> schemas = new ArrayList<>();
+ jsonNode.forEach(node -> schemas.add(node.asText()));
+ return schemas;
+ }
+
+ private String sendGetRequest(String path, @Nullable Map<String, String>
headers)
+ throws IOException {
+ String baseUrl = _helixManager != null ?
getControllerBaseUrl(_helixManager) : _controllerUrl;
+ if (baseUrl == null) {
+ throw new IOException("Controller URL is not configured for metadata
query");
+ }
+ String urlString = baseUrl.endsWith("/") ? baseUrl.substring(0,
baseUrl.length() - 1) + path : baseUrl + path;
Review Comment:
The URL construction logic could be simplified using a utility method or
standard URL building approach. Consider extracting this to a helper method
like `buildUrl(baseUrl, path)` to improve readability and reusability.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/executor/sql/SqlQueryExecutor.java:
##########
@@ -121,11 +143,207 @@ public BrokerResponse
executeDMLStatement(SqlNodeAndOptions sqlNodeAndOptions,
return result;
}
+ /**
+ * Execute metadata (SHOW/USE) statements.
+ */
+ public BrokerResponse executeMetadataStatement(SqlNodeAndOptions
sqlNodeAndOptions,
+ @Nullable Map<String, String> headers) {
+ BrokerResponseNative result = new BrokerResponseNative();
+ try {
+ ResultTable resultTable =
+ buildMetadataResult(sqlNodeAndOptions.getSqlNode(),
sqlNodeAndOptions.getOptions(), headers);
+ result.setResultTable(resultTable);
+ } catch (Exception e) {
+ result.addException(new
QueryProcessingException(QueryErrorCode.QUERY_EXECUTION, e.getMessage()));
+ }
+ return result;
+ }
+
private MinionClient getMinionClient() {
// NOTE: using null auth provider here as auth headers injected by caller
in "executeDMLStatement()"
if (_helixManager != null) {
return new MinionClient(getControllerBaseUrl(_helixManager), null);
}
return new MinionClient(_controllerUrl, null);
}
+
+ private ResultTable buildMetadataResult(SqlNode sqlNode, Map<String, String>
options,
+ @Nullable Map<String, String> headers)
+ throws Exception {
+ if (sqlNode instanceof SqlShowDatabases) {
+ return toSingleStringColumnResult("databaseName",
fetchDatabases(headers));
+ }
+ if (sqlNode instanceof SqlShowTables) {
+ SqlShowTables showTables = (SqlShowTables) sqlNode;
+ String database = resolveDatabase(showTables.getDatabaseName(), options,
headers);
+ List<String> tables = stripDatabasePrefix(fetchTables(database,
headers), database);
+ return toSingleStringColumnResult("tableName", tables);
+ }
+ if (sqlNode instanceof SqlShowSchemas) {
+ SqlShowSchemas showSchemas = (SqlShowSchemas) sqlNode;
+ String database = resolveDatabase(null, options, headers);
+ List<String> schemas = stripDatabasePrefix(fetchSchemas(database,
headers), database);
+ String likePattern = getLikePattern(showSchemas.getLikePattern());
+ schemas = applyLikeFilter(schemas, likePattern);
+ return toSingleStringColumnResult("schemaName", schemas);
+ }
+ throw new UnsupportedOperationException("Unsupported METADATA SqlKind - "
+ sqlNode.getKind());
+ }
+
+ private ResultTable toSingleStringColumnResult(String columnName,
List<String> values) {
+ DataSchema dataSchema = new DataSchema(new String[]{columnName}, new
ColumnDataType[]{ColumnDataType.STRING});
+ List<Object[]> rows = values.stream().map(value -> new
Object[]{value}).collect(Collectors.toList());
+ return new ResultTable(dataSchema, rows);
+ }
+
+ private String resolveDatabase(@Nullable SqlIdentifier explicitDatabase,
Map<String, String> options,
+ @Nullable Map<String, String> headers)
+ throws DatabaseConflictException {
+ String databaseFromSql = explicitDatabase != null ?
explicitDatabase.toString() : null;
+ String databaseFromOptions = options.get(CommonConstants.DATABASE);
+ String databaseFromHeaders = getHeaderValue(headers,
CommonConstants.DATABASE);
+
+ if (databaseFromSql != null) {
+ if (databaseFromOptions != null &&
!databaseFromSql.equalsIgnoreCase(databaseFromOptions)) {
+ throw new DatabaseConflictException(
+ "Database name '" + databaseFromSql + "' from statement does not
match database name '"
+ + databaseFromOptions + "' from query options and database
name '" + databaseFromHeaders
+ + "' from request header");
+ }
+ if (databaseFromHeaders != null &&
!databaseFromSql.equalsIgnoreCase(databaseFromHeaders)) {
+ throw new DatabaseConflictException(
+ "Database name '" + databaseFromSql + "' from statement does not
match database name '"
+ + databaseFromHeaders + "' from request header and database
name '" + databaseFromOptions
+ + "' from query options");
Review Comment:
Similar to the previous error message, the sources mentioned in the error
message ('from request header and database name from query options') are in
reversed order compared to the comparison logic (checking against headers
first, then mentioning options). This inconsistency makes the error message
potentially confusing.
```suggestion
+ databaseFromOptions + "' from query options and database
name '" + databaseFromHeaders
+ "' from request header");
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/executor/sql/SqlQueryExecutor.java:
##########
@@ -121,11 +143,207 @@ public BrokerResponse
executeDMLStatement(SqlNodeAndOptions sqlNodeAndOptions,
return result;
}
+ /**
+ * Execute metadata (SHOW/USE) statements.
+ */
+ public BrokerResponse executeMetadataStatement(SqlNodeAndOptions
sqlNodeAndOptions,
+ @Nullable Map<String, String> headers) {
+ BrokerResponseNative result = new BrokerResponseNative();
+ try {
+ ResultTable resultTable =
+ buildMetadataResult(sqlNodeAndOptions.getSqlNode(),
sqlNodeAndOptions.getOptions(), headers);
+ result.setResultTable(resultTable);
+ } catch (Exception e) {
+ result.addException(new
QueryProcessingException(QueryErrorCode.QUERY_EXECUTION, e.getMessage()));
+ }
+ return result;
+ }
+
private MinionClient getMinionClient() {
// NOTE: using null auth provider here as auth headers injected by caller
in "executeDMLStatement()"
if (_helixManager != null) {
return new MinionClient(getControllerBaseUrl(_helixManager), null);
}
return new MinionClient(_controllerUrl, null);
}
+
+ private ResultTable buildMetadataResult(SqlNode sqlNode, Map<String, String>
options,
+ @Nullable Map<String, String> headers)
+ throws Exception {
+ if (sqlNode instanceof SqlShowDatabases) {
+ return toSingleStringColumnResult("databaseName",
fetchDatabases(headers));
+ }
+ if (sqlNode instanceof SqlShowTables) {
+ SqlShowTables showTables = (SqlShowTables) sqlNode;
+ String database = resolveDatabase(showTables.getDatabaseName(), options,
headers);
+ List<String> tables = stripDatabasePrefix(fetchTables(database,
headers), database);
+ return toSingleStringColumnResult("tableName", tables);
+ }
+ if (sqlNode instanceof SqlShowSchemas) {
+ SqlShowSchemas showSchemas = (SqlShowSchemas) sqlNode;
+ String database = resolveDatabase(null, options, headers);
+ List<String> schemas = stripDatabasePrefix(fetchSchemas(database,
headers), database);
+ String likePattern = getLikePattern(showSchemas.getLikePattern());
+ schemas = applyLikeFilter(schemas, likePattern);
+ return toSingleStringColumnResult("schemaName", schemas);
+ }
+ throw new UnsupportedOperationException("Unsupported METADATA SqlKind - "
+ sqlNode.getKind());
+ }
+
+ private ResultTable toSingleStringColumnResult(String columnName,
List<String> values) {
+ DataSchema dataSchema = new DataSchema(new String[]{columnName}, new
ColumnDataType[]{ColumnDataType.STRING});
+ List<Object[]> rows = values.stream().map(value -> new
Object[]{value}).collect(Collectors.toList());
+ return new ResultTable(dataSchema, rows);
+ }
+
+ private String resolveDatabase(@Nullable SqlIdentifier explicitDatabase,
Map<String, String> options,
+ @Nullable Map<String, String> headers)
+ throws DatabaseConflictException {
+ String databaseFromSql = explicitDatabase != null ?
explicitDatabase.toString() : null;
+ String databaseFromOptions = options.get(CommonConstants.DATABASE);
+ String databaseFromHeaders = getHeaderValue(headers,
CommonConstants.DATABASE);
+
+ if (databaseFromSql != null) {
+ if (databaseFromOptions != null &&
!databaseFromSql.equalsIgnoreCase(databaseFromOptions)) {
+ throw new DatabaseConflictException(
+ "Database name '" + databaseFromSql + "' from statement does not
match database name '"
+ + databaseFromOptions + "' from query options and database
name '" + databaseFromHeaders
+ + "' from request header");
Review Comment:
The error message mentions 'database name from request header' but
`databaseFromOptions` is referenced before `databaseFromHeaders` in the
concatenation. The order in the message should match the comparison logic or be
clarified to avoid confusion about which sources are being compared.
```suggestion
+ databaseFromHeaders + "' from request header and database
name '" + databaseFromOptions
+ "' from query options");
```
##########
pinot-common/src/main/codegen/config.fmpp:
##########
@@ -44,6 +44,9 @@ data: {
keywords: [
"FILE"
"ARCHIVE"
+ "DATABASES"
+ "TABLES"
+ "SCHEMAS"
Review Comment:
The keywords "DATABASES", "TABLES", and "SCHEMAS" are defined twice in this
configuration file (lines 47-49 and lines 76-78). This duplication could lead
to maintenance issues if these keywords need to be updated in the future.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
