Copilot commented on code in PR #16746:
URL: https://github.com/apache/pinot/pull/16746#discussion_r2324062581
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -160,54 +160,67 @@ public StreamingOutput
handleTimeSeriesQueryRange(@QueryParam("language") String
@POST
@Path("validateMultiStageQuery")
- public MultiStageQueryValidationResponse
validateMultiStageQuery(MultiStageQueryValidationRequest request,
+ public List<MultiStageQueryValidationResponse>
validateMultiStageQuery(MultiStageQueryValidationRequest request,
@Context HttpHeaders httpHeaders) {
- String sqlQuery = request.getSql().trim();
- if (request.getSql() == null || sqlQuery.isEmpty()) {
- return new MultiStageQueryValidationResponse(false, "Request is missing
the query string field 'sql'", null);
- }
-
- Map<String, String> queryOptionsMap =
RequestUtils.parseQuery(sqlQuery).getOptions();
- String database =
DatabaseUtils.extractDatabaseFromQueryRequest(queryOptionsMap, httpHeaders);
-
- try {
- TableCache tableCache;
- if (CollectionUtils.isNotEmpty(request.getTableConfigs()) &&
CollectionUtils.isNotEmpty(request.getSchemas())) {
- tableCache =
- new StaticTableCache(request.getTableConfigs(),
request.getSchemas(), request.getLogicalTableConfigs(),
- request.isIgnoreCase());
- LOGGER.info("Validating multi-stage query compilation using static
table cache for query: {}",
- request.getSql());
- } else {
- // Use TableCache from environment if static fields are not specified
- tableCache = _pinotHelixResourceManager.getTableCache();
- LOGGER.info("Validating multi-stage query compilation using Zk table
cache for query: {}", request.getSql());
- }
- try (QueryEnvironment.CompiledQuery compiledQuery = new
QueryEnvironment(database, tableCache, null).compile(
- sqlQuery)) {
- return new MultiStageQueryValidationResponse(true, null, null);
+ List<String> sqlQueries = request.getSql();
+ List<MultiStageQueryValidationResponse> multiStageQueryValidationResponses
= new ArrayList<>();
+ if (request.getSql() == null || sqlQueries.isEmpty()) {
+ MultiStageQueryValidationResponse multiStageQueryValidationResponse =
+ new MultiStageQueryValidationResponse(false, "Request is missing the
queries string field 'sql'", null, null);
+
multiStageQueryValidationResponses.add(multiStageQueryValidationResponse);
Review Comment:
When the SQL query list is null or empty, the method adds an error response
but continues processing the (empty) list, which will result in returning the
error response plus attempting to iterate over the empty list. This should
return early after adding the error response.
```suggestion
multiStageQueryValidationResponses.add(multiStageQueryValidationResponse);
return multiStageQueryValidationResponses;
```
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java:
##########
@@ -1880,11 +1971,6 @@ public void testValidateQueryApiWithStaticTable()
upsertConfig.setEnablePreload(true);
upsertConfig.setDropOutOfOrderRecord(false);
upsertConfig.setNewSegmentTrackingTimeMs(10000L);
-
upsertConfig.setMetadataManagerClass("ai.startree.pinot.upsert.rocksdb.RocksDBTableUpsertMetadataManager");
-
- Map<String, String> metadataManagerConfigs = new HashMap<>();
- metadataManagerConfigs.put("rocksdb.preload.num_partition_overwrite", "2");
- upsertConfig.setMetadataManagerConfigs(metadataManagerConfigs);
upsertConfig.setDefaultPartialUpsertStrategy(UpsertConfig.Strategy.OVERWRITE);
Review Comment:
The removed lines containing `setMetadataManagerClass` and
`setMetadataManagerConfigs` appear to be unrelated to the main purpose of this
PR (adding multiple query support). These changes should be in a separate
commit or PR to keep the diff focused on the stated purpose.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -160,54 +160,67 @@ public StreamingOutput
handleTimeSeriesQueryRange(@QueryParam("language") String
@POST
@Path("validateMultiStageQuery")
- public MultiStageQueryValidationResponse
validateMultiStageQuery(MultiStageQueryValidationRequest request,
+ public List<MultiStageQueryValidationResponse>
validateMultiStageQuery(MultiStageQueryValidationRequest request,
@Context HttpHeaders httpHeaders) {
- String sqlQuery = request.getSql().trim();
- if (request.getSql() == null || sqlQuery.isEmpty()) {
- return new MultiStageQueryValidationResponse(false, "Request is missing
the query string field 'sql'", null);
- }
-
- Map<String, String> queryOptionsMap =
RequestUtils.parseQuery(sqlQuery).getOptions();
- String database =
DatabaseUtils.extractDatabaseFromQueryRequest(queryOptionsMap, httpHeaders);
-
- try {
- TableCache tableCache;
- if (CollectionUtils.isNotEmpty(request.getTableConfigs()) &&
CollectionUtils.isNotEmpty(request.getSchemas())) {
- tableCache =
- new StaticTableCache(request.getTableConfigs(),
request.getSchemas(), request.getLogicalTableConfigs(),
- request.isIgnoreCase());
- LOGGER.info("Validating multi-stage query compilation using static
table cache for query: {}",
- request.getSql());
- } else {
- // Use TableCache from environment if static fields are not specified
- tableCache = _pinotHelixResourceManager.getTableCache();
- LOGGER.info("Validating multi-stage query compilation using Zk table
cache for query: {}", request.getSql());
- }
- try (QueryEnvironment.CompiledQuery compiledQuery = new
QueryEnvironment(database, tableCache, null).compile(
- sqlQuery)) {
- return new MultiStageQueryValidationResponse(true, null, null);
+ List<String> sqlQueries = request.getSql();
+ List<MultiStageQueryValidationResponse> multiStageQueryValidationResponses
= new ArrayList<>();
+ if (request.getSql() == null || sqlQueries.isEmpty()) {
+ MultiStageQueryValidationResponse multiStageQueryValidationResponse =
+ new MultiStageQueryValidationResponse(false, "Request is missing the
queries string field 'sql'", null, null);
+
multiStageQueryValidationResponses.add(multiStageQueryValidationResponse);
+ }
+ for (String sqlQuery : sqlQueries) {
+ Map<String, String> queryOptionsMap =
RequestUtils.parseQuery(sqlQuery).getOptions();
+ String database =
DatabaseUtils.extractDatabaseFromQueryRequest(queryOptionsMap, httpHeaders);
+
+ try {
+ TableCache tableCache;
+ if (CollectionUtils.isNotEmpty(request.getTableConfigs()) &&
CollectionUtils.isNotEmpty(request.getSchemas())) {
+ tableCache =
+ new StaticTableCache(request.getTableConfigs(),
request.getSchemas(), request.getLogicalTableConfigs(),
+ request.isIgnoreCase());
+ LOGGER.info("Validating multi-stage query: {} compilation using
static table cache ",
+ request.getSql());
+ } else {
+ // Use TableCache from environment if static fields are not specified
+ tableCache = _pinotHelixResourceManager.getTableCache();
+ LOGGER.info("Validating multi-stage query: {} compilation using Zk
table cache", request.getSql());
Review Comment:
The log message logs the entire list of SQL queries instead of the current
query being processed. This should log `sqlQuery` instead of `request.getSql()`
to show which specific query is being validated.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -160,54 +160,67 @@ public StreamingOutput
handleTimeSeriesQueryRange(@QueryParam("language") String
@POST
@Path("validateMultiStageQuery")
- public MultiStageQueryValidationResponse
validateMultiStageQuery(MultiStageQueryValidationRequest request,
+ public List<MultiStageQueryValidationResponse>
validateMultiStageQuery(MultiStageQueryValidationRequest request,
@Context HttpHeaders httpHeaders) {
- String sqlQuery = request.getSql().trim();
- if (request.getSql() == null || sqlQuery.isEmpty()) {
- return new MultiStageQueryValidationResponse(false, "Request is missing
the query string field 'sql'", null);
- }
-
- Map<String, String> queryOptionsMap =
RequestUtils.parseQuery(sqlQuery).getOptions();
- String database =
DatabaseUtils.extractDatabaseFromQueryRequest(queryOptionsMap, httpHeaders);
-
- try {
- TableCache tableCache;
- if (CollectionUtils.isNotEmpty(request.getTableConfigs()) &&
CollectionUtils.isNotEmpty(request.getSchemas())) {
- tableCache =
- new StaticTableCache(request.getTableConfigs(),
request.getSchemas(), request.getLogicalTableConfigs(),
- request.isIgnoreCase());
- LOGGER.info("Validating multi-stage query compilation using static
table cache for query: {}",
- request.getSql());
- } else {
- // Use TableCache from environment if static fields are not specified
- tableCache = _pinotHelixResourceManager.getTableCache();
- LOGGER.info("Validating multi-stage query compilation using Zk table
cache for query: {}", request.getSql());
- }
- try (QueryEnvironment.CompiledQuery compiledQuery = new
QueryEnvironment(database, tableCache, null).compile(
- sqlQuery)) {
- return new MultiStageQueryValidationResponse(true, null, null);
+ List<String> sqlQueries = request.getSql();
+ List<MultiStageQueryValidationResponse> multiStageQueryValidationResponses
= new ArrayList<>();
+ if (request.getSql() == null || sqlQueries.isEmpty()) {
+ MultiStageQueryValidationResponse multiStageQueryValidationResponse =
+ new MultiStageQueryValidationResponse(false, "Request is missing the
queries string field 'sql'", null, null);
+
multiStageQueryValidationResponses.add(multiStageQueryValidationResponse);
+ }
+ for (String sqlQuery : sqlQueries) {
+ Map<String, String> queryOptionsMap =
RequestUtils.parseQuery(sqlQuery).getOptions();
+ String database =
DatabaseUtils.extractDatabaseFromQueryRequest(queryOptionsMap, httpHeaders);
+
+ try {
+ TableCache tableCache;
+ if (CollectionUtils.isNotEmpty(request.getTableConfigs()) &&
CollectionUtils.isNotEmpty(request.getSchemas())) {
+ tableCache =
+ new StaticTableCache(request.getTableConfigs(),
request.getSchemas(), request.getLogicalTableConfigs(),
+ request.isIgnoreCase());
+ LOGGER.info("Validating multi-stage query: {} compilation using
static table cache ",
+ request.getSql());
+ } else {
+ // Use TableCache from environment if static fields are not specified
+ tableCache = _pinotHelixResourceManager.getTableCache();
+ LOGGER.info("Validating multi-stage query: {} compilation using Zk
table cache", request.getSql());
Review Comment:
Same issue as above - this logs the entire list of SQL queries instead of
the current query being processed. Should log `sqlQuery` instead of
`request.getSql()`.
```suggestion
LOGGER.info("Validating multi-stage query: {} compilation using Zk
table cache", sqlQuery);
```
--
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]
