This is an automated email from the ASF dual-hosted git repository.
gortiz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 2ac1db8d01 Fix error message on database context mismatch in v1 query
(#12699)
2ac1db8d01 is described below
commit 2ac1db8d01cafdfd869d3dfb9f18c5277c2ad45f
Author: Shounak kulkarni <[email protected]>
AuthorDate: Mon Mar 25 13:14:55 2024 +0500
Fix error message on database context mismatch in v1 query (#12699)
* Fix error message on database context mismatch in v1 query
* Handle database context conflicts gracefully
* test fix
* test fix
---
.../requesthandler/BaseBrokerRequestHandler.java | 14 ++++--
.../MultiStageBrokerRequestHandler.java | 6 +++
.../apache/pinot/common/utils/DatabaseUtils.java | 50 ++++++++++++++++------
.../pinot/common/utils/DatabaseUtilsTest.java | 3 +-
.../api/resources/PinotQueryResource.java | 9 ++--
.../tests/MultiStageEngineIntegrationTest.java | 14 +++---
.../spi/exception/DatabaseConflictException.java | 25 +++++++++++
7 files changed, 94 insertions(+), 27 deletions(-)
diff --git
a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
index fa30f3c5bf..31bae2215a 100644
---
a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
+++
b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
@@ -95,6 +95,7 @@ import org.apache.pinot.spi.env.PinotConfiguration;
import org.apache.pinot.spi.eventlistener.query.BrokerQueryEventListener;
import
org.apache.pinot.spi.eventlistener.query.PinotBrokerQueryEventListenerFactory;
import org.apache.pinot.spi.exception.BadQueryRequestException;
+import org.apache.pinot.spi.exception.DatabaseConflictException;
import org.apache.pinot.spi.trace.RequestContext;
import org.apache.pinot.spi.trace.Tracing;
import org.apache.pinot.spi.utils.BigDecimalUtils;
@@ -375,9 +376,16 @@ public abstract class BaseBrokerRequestHandler implements
BrokerRequestHandler {
}
boolean ignoreCase = _tableCache.isIgnoreCase();
- String tableName =
-
getActualTableName(DatabaseUtils.translateTableName(dataSource.getTableName(),
httpHeaders, ignoreCase),
- _tableCache);
+ String tableName;
+ try {
+ tableName = getActualTableName(
+ DatabaseUtils.translateTableName(dataSource.getTableName(),
httpHeaders, ignoreCase), _tableCache);
+ } catch (DatabaseConflictException e) {
+ LOGGER.info("{}. Request {}: {}", e.getMessage(), requestId, query);
+
_brokerMetrics.addMeteredGlobalValue(BrokerMeter.QUERY_VALIDATION_EXCEPTIONS,
1);
+
requestContext.setErrorCode(QueryException.QUERY_VALIDATION_ERROR_CODE);
+ return new
BrokerResponseNative(QueryException.getException(QueryException.QUERY_VALIDATION_ERROR,
e));
+ }
dataSource.setTableName(tableName);
String rawTableName = TableNameBuilder.extractRawTableName(tableName);
requestContext.setTableName(rawTableName);
diff --git
a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java
b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java
index 7f856b6a54..ee9f6cf19a 100644
---
a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java
+++
b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java
@@ -71,6 +71,7 @@ import org.apache.pinot.query.type.TypeFactory;
import org.apache.pinot.query.type.TypeSystem;
import org.apache.pinot.spi.env.PinotConfiguration;
import org.apache.pinot.spi.eventlistener.query.BrokerQueryEventListener;
+import org.apache.pinot.spi.exception.DatabaseConflictException;
import org.apache.pinot.spi.trace.RequestContext;
import org.apache.pinot.spi.utils.CommonConstants;
import org.apache.pinot.spi.utils.builder.TableNameBuilder;
@@ -146,6 +147,11 @@ public class MultiStageBrokerRequestHandler extends
BaseBrokerRequestHandler {
queryPlanResult = queryEnvironment.planQuery(query,
sqlNodeAndOptions, requestId);
break;
}
+ } catch (DatabaseConflictException e) {
+ LOGGER.info("{}. Request {}: {}", e.getMessage(), requestId, query);
+
_brokerMetrics.addMeteredGlobalValue(BrokerMeter.QUERY_VALIDATION_EXCEPTIONS,
1);
+ requestContext.setErrorCode(QueryException.QUERY_VALIDATION_ERROR_CODE);
+ return new
BrokerResponseNative(QueryException.getException(QueryException.QUERY_VALIDATION_ERROR,
e));
} catch (WebApplicationException e) {
throw e;
} catch (RuntimeException e) {
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/utils/DatabaseUtils.java
b/pinot-common/src/main/java/org/apache/pinot/common/utils/DatabaseUtils.java
index 6913c409a6..28c9235cd9 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/utils/DatabaseUtils.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/utils/DatabaseUtils.java
@@ -24,6 +24,7 @@ import java.util.Objects;
import javax.annotation.Nullable;
import javax.ws.rs.core.HttpHeaders;
import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.spi.exception.DatabaseConflictException;
import org.apache.pinot.spi.utils.CommonConstants;
@@ -37,8 +38,10 @@ public class DatabaseUtils {
* @param databaseName database name
* @param ignoreCase whether to ignore case when comparing passed in
database name against table name prefix if both
* exist. For 'default' database, always compare it
ignoring case.
- * @return translated table name. Throws {@link IllegalArgumentException} if
{@code tableName} contains
- * more than 1 dot or if {@code tableName} has database prefix, and it does
not match with {@code databaseName}
+ * @return translated table name.
+ * <br>Throws {@link IllegalArgumentException} if {@code tableName} contains
more than 1 dot
+ * <br>Throws {@link DatabaseConflictException} if {@code tableName} has
database prefix,
+ * and it does not match with {@code databaseName}
*/
public static String translateTableName(String tableName, @Nullable String
databaseName, boolean ignoreCase) {
Preconditions.checkArgument(StringUtils.isNotEmpty(tableName),
"'tableName' cannot be null or empty");
@@ -53,11 +56,11 @@ public class DatabaseUtils {
case 2:
Preconditions.checkArgument(!tableSplit[1].isEmpty(), "Invalid table
name '%s'", tableName);
String databasePrefix = tableSplit[0];
- Preconditions.checkArgument(
- StringUtils.isEmpty(databaseName) || (!ignoreCase &&
databaseName.equals(databasePrefix)) || (ignoreCase
- && databaseName.equalsIgnoreCase(databasePrefix)),
- "Database name '%s' from table prefix does not match database name
'%s' from header", databasePrefix,
- databaseName);
+ if (!StringUtils.isEmpty(databaseName) && (ignoreCase ||
!databaseName.equals(databasePrefix))
+ && (!ignoreCase ||
!databaseName.equalsIgnoreCase(databasePrefix))) {
+ throw new DatabaseConflictException("Database name '" +
databasePrefix
+ + "' from table prefix does not match database name '" +
databaseName + "' from header");
+ }
// skip database name prefix if it's a 'default' database
return
databasePrefix.equalsIgnoreCase(CommonConstants.DEFAULT_DATABASE) ?
tableSplit[1] : tableName;
default:
@@ -66,6 +69,15 @@ public class DatabaseUtils {
}
}
+ /**
+ * Construct the fully qualified table name i.e. {databaseName}.{tableName}
from given table name and database name
+ * @param tableName table/schema name
+ * @param databaseName database name
+ * @return translated table name.
+ * <br>Throws {@link IllegalArgumentException} if {@code tableName} contains
more than 1 dot
+ * <br>Throws {@link DatabaseConflictException} if {@code tableName} has
database prefix,
+ * and it does not match with {@code databaseName}
+ */
public static String translateTableName(String tableName, @Nullable String
databaseName) {
return translateTableName(tableName, databaseName, false);
}
@@ -76,13 +88,24 @@ public class DatabaseUtils {
* @param headers http headers
* @param ignoreCase whether to ignore case when comparing database name in
headers against table name prefix if both
* exist. For 'default' database, always compare it
ignoring case.
- * @return translated table name. Throws {@link IllegalStateException} if
{@code tableName} contains more than 1 dot
- * or if {@code tableName} has database prefix, and it does not match with
the 'database' header
+ * @return translated table name.
+ * <br>Throws {@link IllegalArgumentException} if {@code tableName} contains
more than 1 dot
+ * <br>Throws {@link DatabaseConflictException} if {@code tableName} has
database prefix,
+ * and it does not match with the 'database' header
*/
public static String translateTableName(String tableName, HttpHeaders
headers, boolean ignoreCase) {
return translateTableName(tableName,
headers.getHeaderString(CommonConstants.DATABASE), ignoreCase);
}
+ /**
+ * Utility to get fully qualified table name i.e. {databaseName}.{tableName}
from given table name and http headers
+ * @param tableName table/schema name
+ * @param headers http headers
+ * @return translated table name.
+ * <br>Throws {@link IllegalArgumentException} if {@code tableName} contains
more than 1 dot
+ * <br>Throws {@link DatabaseConflictException} if {@code tableName} has
database prefix,
+ * and it does not match with the 'database' header
+ */
public static String translateTableName(String tableName, HttpHeaders
headers) {
return translateTableName(tableName, headers, false);
}
@@ -117,15 +140,16 @@ public class DatabaseUtils {
* @param headers http headers from request
* @return extracted database name.
* <br>If database context is not provided at all return {@link
CommonConstants#DEFAULT_DATABASE}.
- * <br>If queryOptions and headers have conflicting database context an
{@link IllegalArgumentException} is thrown.
+ * <br>If queryOptions and headers have conflicting database context an
{@link DatabaseConflictException} is thrown.
*/
public static String extractDatabaseFromQueryRequest(
@Nullable Map<String, String> queryOptions, @Nullable HttpHeaders
headers) {
String databaseFromOptions = queryOptions == null ? null :
queryOptions.get(CommonConstants.DATABASE);
String databaseFromHeaders = headers == null ? null :
headers.getHeaderString(CommonConstants.DATABASE);
- if (databaseFromHeaders != null && databaseFromOptions != null) {
-
Preconditions.checkArgument(databaseFromOptions.equals(databaseFromHeaders),
"Database context mismatch : "
- + "from headers %s, from query options %s", databaseFromHeaders,
databaseFromOptions);
+ if (databaseFromHeaders != null && databaseFromOptions != null
+ && !databaseFromOptions.equals(databaseFromHeaders)) {
+ throw new DatabaseConflictException("Database context mismatch : from
headers '" + databaseFromHeaders
+ + "', from query options '" + databaseFromOptions + "'");
}
String database = databaseFromHeaders != null ? databaseFromHeaders :
databaseFromOptions;
return Objects.requireNonNullElse(database,
CommonConstants.DEFAULT_DATABASE);
diff --git
a/pinot-common/src/test/java/org/apache/pinot/common/utils/DatabaseUtilsTest.java
b/pinot-common/src/test/java/org/apache/pinot/common/utils/DatabaseUtilsTest.java
index edbd618702..5036641619 100644
---
a/pinot-common/src/test/java/org/apache/pinot/common/utils/DatabaseUtilsTest.java
+++
b/pinot-common/src/test/java/org/apache/pinot/common/utils/DatabaseUtilsTest.java
@@ -18,6 +18,7 @@
*/
package org.apache.pinot.common.utils;
+import org.apache.pinot.spi.exception.DatabaseConflictException;
import org.apache.pinot.spi.utils.CommonConstants;
import org.testng.annotations.Test;
@@ -72,7 +73,7 @@ public class DatabaseUtilsTest {
try {
DatabaseUtils.translateTableName(tableName, databaseName);
fail();
- } catch (IllegalArgumentException ignored) {
+ } catch (IllegalArgumentException | DatabaseConflictException ignored) {
return;
}
}
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
index b9f13a53f4..3d8a3bb263 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
@@ -73,6 +73,7 @@ import org.apache.pinot.query.catalog.PinotCatalog;
import org.apache.pinot.query.type.TypeFactory;
import org.apache.pinot.query.type.TypeSystem;
import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.exception.DatabaseConflictException;
import org.apache.pinot.spi.utils.CommonConstants;
import
org.apache.pinot.spi.utils.CommonConstants.Broker.Request.QueryOptionKey;
import org.apache.pinot.spi.utils.JsonUtils;
@@ -253,13 +254,15 @@ public class PinotQueryResource {
if (queryOptions != null) {
queryOptionsMap.putAll(RequestUtils.getOptionsFromString(queryOptions));
}
- String database =
DatabaseUtils.extractDatabaseFromQueryRequest(queryOptionsMap, httpHeaders);
+ String database = null;
try {
String inputTableName =
sqlNode != null ?
RequestUtils.getTableNames(CalciteSqlParser.compileSqlNodeToPinotQuery(sqlNode)).iterator()
.next() :
CalciteSqlCompiler.compileToBrokerRequest(query).getQuerySource().getTableName();
- tableName = _pinotHelixResourceManager.getActualTableName(inputTableName,
- httpHeaders.getHeaderString(CommonConstants.DATABASE));
+ database =
DatabaseUtils.extractDatabaseFromQueryRequest(queryOptionsMap, httpHeaders);
+ tableName =
_pinotHelixResourceManager.getActualTableName(inputTableName, database);
+ } catch (DatabaseConflictException e) {
+ return
QueryException.getException(QueryException.QUERY_VALIDATION_ERROR,
e).toString();
} catch (Exception e) {
LOGGER.error("Caught exception while compiling query: {}", query, e);
try {
diff --git
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
index 0f5c5f611e..33825ef727 100644
---
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
+++
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
@@ -849,7 +849,7 @@ public class MultiStageEngineIntegrationTest extends
BaseClusterIntegrationTestS
// Using renamed column "ActualElapsedTime_2" to ensure that the same
table is not being queried.
// custom database check. Database context passed only as table prefix.
Will
JsonNode result = getQueryResultForDBTest("ActualElapsedTime_2",
TABLE_NAME_WITH_DATABASE, null, null);
- checkQueryPlanningErrorForDBTest(result);
+ checkQueryPlanningErrorForDBTest(result,
QueryException.QUERY_PLANNING_ERROR_CODE);
}
@Test
@@ -891,7 +891,7 @@ public class MultiStageEngineIntegrationTest extends
BaseClusterIntegrationTestS
throws Exception {
JsonNode result = getQueryResultForDBTest("ActualElapsedTime",
TABLE_NAME_WITH_DATABASE, DEFAULT_DATABASE_NAME,
null);
- checkQueryPlanningErrorForDBTest(result);
+ checkQueryPlanningErrorForDBTest(result,
QueryException.QUERY_PLANNING_ERROR_CODE);
}
@Test
@@ -899,7 +899,7 @@ public class MultiStageEngineIntegrationTest extends
BaseClusterIntegrationTestS
throws Exception {
JsonNode result = getQueryResultForDBTest("ActualElapsedTime",
TABLE_NAME_WITH_DATABASE, null,
Collections.singletonMap(CommonConstants.DATABASE,
DEFAULT_DATABASE_NAME));
- checkQueryPlanningErrorForDBTest(result);
+ checkQueryPlanningErrorForDBTest(result,
QueryException.QUERY_PLANNING_ERROR_CODE);
}
@Test
@@ -907,7 +907,7 @@ public class MultiStageEngineIntegrationTest extends
BaseClusterIntegrationTestS
throws Exception {
JsonNode result = getQueryResultForDBTest("ActualElapsedTime",
TABLE_NAME_WITH_DATABASE, DATABASE_NAME,
Collections.singletonMap(CommonConstants.DATABASE,
DEFAULT_DATABASE_NAME));
- checkQueryPlanningErrorForDBTest(result);
+ checkQueryPlanningErrorForDBTest(result,
QueryException.QUERY_VALIDATION_ERROR_CODE);
}
@Test
@@ -918,7 +918,7 @@ public class MultiStageEngineIntegrationTest extends
BaseClusterIntegrationTestS
+ " Carrier FROM " + TABLE_NAME_WITH_DATABASE + " GROUP BY Carrier) AS
tb2 "
+ "ON tb1.Carrier = tb2.Carrier; ";
JsonNode result = postQuery(query);
- checkQueryPlanningErrorForDBTest(result);
+ checkQueryPlanningErrorForDBTest(result,
QueryException.QUERY_PLANNING_ERROR_CODE);
}
private void checkQueryResultForDBTest(String column, String tableName)
@@ -946,9 +946,9 @@ public class MultiStageEngineIntegrationTest extends
BaseClusterIntegrationTestS
assertEquals(result, expectedValue);
}
- private void checkQueryPlanningErrorForDBTest(JsonNode queryResult) {
+ private void checkQueryPlanningErrorForDBTest(JsonNode queryResult, int
errorCode) {
long result =
queryResult.get("exceptions").get(0).get("errorCode").asInt();
- assertEquals(result, QueryException.QUERY_PLANNING_ERROR_CODE);
+ assertEquals(result, errorCode);
}
private JsonNode getQueryResultForDBTest(String column, String tableName,
@Nullable String database,
diff --git
a/pinot-spi/src/main/java/org/apache/pinot/spi/exception/DatabaseConflictException.java
b/pinot-spi/src/main/java/org/apache/pinot/spi/exception/DatabaseConflictException.java
new file mode 100644
index 0000000000..37c0b82bd4
--- /dev/null
+++
b/pinot-spi/src/main/java/org/apache/pinot/spi/exception/DatabaseConflictException.java
@@ -0,0 +1,25 @@
+/**
+ * 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.spi.exception;
+
+public class DatabaseConflictException extends BadQueryRequestException {
+ public DatabaseConflictException(String message) {
+ super(message);
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
