xiangfu0 commented on code in PR #19664:
URL: https://github.com/apache/pinot/pull/19664#discussion_r4201465637
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -713,6 +738,112 @@ private BrokerResponse executeSqlQuery(ObjectNode
sqlRequestJson, HttpRequesterI
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table.
+ ///
+ /// The first-step access control runs first, as for queries. The table is
then resolved with the database of the
+ /// request and in the case it is defined with, the caller is authorized to
delete rows from it (see
+ /// [#authorizeDelete]), and the executor deletes rows from that exact table.
+ private BrokerResponse executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
Map<String, String> headers,
+ HttpRequesterIdentity requesterIdentity, @Nullable HttpHeaders
httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ // The first-step access control runs before the table is looked up, as
for queries
+ AuthorizationResult authorizationResult =
accessControl.authorize(requesterIdentity);
+ if (!authorizationResult.hasAccess()) {
+ throw deleteAccessDenied(null, authorizationResult);
+ }
+ if (_tableCache == null) {
+ return new BrokerResponseNative(QueryErrorCode.QUERY_VALIDATION,
+ "DELETE is not supported by this broker: no table cache was
configured");
+ }
+ DeleteStatement statement;
+ try {
+ String databaseHeader = httpHeaders != null ?
httpHeaders.getHeaderString(CommonConstants.DATABASE) : null;
+ statement = ((DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions))
+ .resolveTableName(databaseHeader, _tableCache);
+ } catch (QueryException e) {
+ // e.g. an invalid statement, a logical table, or a database header that
does not match the statement
+ return new BrokerResponseNative(e.getErrorCode(), e.getMessage());
+ }
+ authorizeDelete(accessControl, statement.getTableName(),
requesterIdentity, httpHeaders);
+ return _sqlQueryExecutor.executeStatement(statement, headers);
Review Comment:
Both entry points now log the resolved table, predicate, option keys and
client (the requester IP on the broker, the proxy headers on the controller) at
INFO right before the executor call (d5065b2950). I left the counter out of
this PR to keep the metric surface unchanged; happy to add a meter next to the
denial one in a follow-up if we want it.
##########
pinot-broker/src/test/java/org/apache/pinot/broker/broker/BasicAuthAccessControlTest.java:
##########
@@ -199,4 +214,44 @@ public void testNormalizeToken() {
Assert.assertTrue(_accessControl.authorize(identity, request).hasAccess());
Assert.assertTrue(_accessControl.authorize(identity,
_tableNames).hasAccess());
}
+
+ @Test
+ public void testDeleteRowsRequiresTheDeletePermission() {
Review Comment:
Added in d5065b2950: a `star` principal with `permissions=*` asserted denied
with "is not granted the DELETE permission", and an `upperDeleter` principal
with `permissions=read,DELETE` asserted allowed.
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/dml/DeleteStatement.java:
##########
@@ -0,0 +1,307 @@
+/**
+ * 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.dml;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.calcite.avatica.util.Casing;
+import org.apache.calcite.sql.SqlDelete;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlSyntax;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.util.SqlShuttle;
+import org.apache.calcite.sql.validate.SqlValidatorUtil;
+import org.apache.pinot.common.config.provider.TableCache;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DatabaseUtils;
+import org.apache.pinot.spi.config.task.AdhocTaskConfig;
+import org.apache.pinot.spi.exception.QueryErrorCode;
+import org.apache.pinot.spi.exception.QueryException;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.apache.pinot.sql.parsers.SqlNodeAndOptions;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+
+/// A SQL `DELETE FROM <table> WHERE <predicate>` statement.
+///
+/// Pinot parses `DELETE` but does not delete rows itself: the default SQL
executor answers that it is not supported,
+/// and a deployment that can delete rows, e.g. by purging the matching rows
from the segments with a minion task,
+/// executes the parsed statement by overriding
`SqlQueryExecutor#executeDelete`. The generic [#execute()] and
+/// [#generateAdhocTaskConfig()] do not apply to it.
+///
+/// Before handing the statement to the executor, the broker and the
controller resolve its table with
+/// [#resolveTableName] and authorize the caller to delete rows from that
table.
+///
+/// Its options are the `SET` statements, the legacy `OPTION(...)` suffix and
the request `queryOptions` of the
+/// statement (`SET` takes precedence). The `database` option only qualifies
the table name, see [#resolveTableName].
+/// Every other option, including query options such as `timeoutMs`, reaches
the executor as written through
+/// [#getOptions()], so that an option the executor relies on (e.g. a dry run)
cannot be reclassified as a query option
+/// and silently dropped.
+///
+/// Instances are immutable and thread-safe.
+public class DeleteStatement implements DataManipulationStatement {
+ public static final String NOT_SUPPORTED_MESSAGE =
+ "DELETE is not supported by this Pinot cluster, it requires a SQL
executor that implements row deletion";
+
+ /// Serializes the WHERE clause into SQL that the Pinot parser reads back
into the same expression. Combined with
+ /// `quoteAllIdentifiers = false`, identifiers are only quoted (with double
quotes) when they were quoted.
+ private static final SqlDialect PINOT_SQL_DIALECT = new
SqlDialect(SqlDialect.EMPTY_CONTEXT
+ .withIdentifierQuoteString("\"")
+ .withLiteralQuoteString("'")
+ .withLiteralEscapedQuoteString("''")
+ .withUnquotedCasing(Casing.UNCHANGED)
+ .withQuotedCasing(Casing.UNCHANGED)
+ .withCaseSensitive(true));
+
+ /// Quotes the unquoted identifiers named like a SQL function without
arguments, e.g. `user`, `pi` or `current_date`:
+ /// Calcite unparses them as upper-cased keywords, while Pinot reads them as
columns, so they would name another
+ /// column. Mirrors the check of `SqlUtil.unparseSqlIdentifierSyntax`.
+ private static final SqlShuttle KEYWORD_IDENTIFIER_QUOTER = new SqlShuttle()
{
+ @Override
+ public SqlNode visit(SqlIdentifier identifier) {
+ if (identifier.isSimple() && !identifier.getParserPosition().isQuoted())
{
+ SqlOperator operator =
+
SqlValidatorUtil.lookupSqlFunctionByID(SqlStdOperatorTable.instance(),
identifier, null);
+ if (operator != null && (operator.getSyntax() == SqlSyntax.FUNCTION_ID
+ || operator.getSyntax() == SqlSyntax.FUNCTION_ID_CONSTANT)) {
+ return new SqlIdentifier(identifier.names, null,
SqlParserPos.QUOTED_ZERO,
+ List.of(SqlParserPos.QUOTED_ZERO));
+ }
+ }
+ return identifier;
+ }
+ };
+
+ /// Canonical names (lower case, without underscores) of the functions that
read another table than the one the
+ /// statement deletes from, which the caller is not authorized to read.
+ private static final Set<String> CROSS_TABLE_FUNCTIONS = Set.of("lookup",
"insubquery", "inpartitionedsubquery");
+
+ private final String _tableName;
+ private final String _predicate;
+ @Nullable
+ private final String _database;
+ private final Map<String, String> _options;
+ private final boolean _resolved;
+
+ private DeleteStatement(String tableName, String predicate, @Nullable String
database, Map<String, String> options) {
+ this(tableName, predicate, database, options, false);
+ }
+
+ private DeleteStatement(String tableName, String predicate, @Nullable String
database, Map<String, String> options,
+ boolean resolved) {
+ _tableName = tableName;
+ _predicate = predicate;
+ _database = database;
+ _options = Collections.unmodifiableMap(new HashMap<>(options));
+ _resolved = resolved;
+ }
+
+ /// Parses a `DELETE` statement.
+ ///
+ /// @throws IllegalArgumentException if the statement is not a supported
`DELETE`: it must have a WHERE clause, no
+ /// table alias, a WHERE clause that Pinot
can parse as an expression and that does
+ /// not read another table (e.g. with
`lookUp` or `IN_SUBQUERY`), and set the
+ /// database with the `database` option only
+ public static DeleteStatement parse(SqlNodeAndOptions sqlNodeAndOptions) {
+ SqlNode sqlNode = sqlNodeAndOptions.getSqlNode();
+ checkArgument(sqlNode instanceof SqlDelete, "Not a DELETE statement: %s",
sqlNode.getKind());
+ SqlDelete sqlDelete = (SqlDelete) sqlNode;
+ String tableName = getTableName(sqlDelete.getTargetTable());
+ checkArgument(sqlDelete.getAlias() == null,
+ "DELETE does not support a table alias, reference the columns
directly");
+ SqlNode condition = sqlDelete.getCondition();
+ checkArgument(condition != null,
+ "DELETE requires a WHERE clause; delete the table segments to remove
all of its rows");
+ String predicate = toPinotSql(condition);
+
+ String database = null;
+ Map<String, String> options = new HashMap<>();
+ for (Map.Entry<String, String> option :
sqlNodeAndOptions.getOptions().entrySet()) {
+ String key = option.getKey();
+ if (key.equals(CommonConstants.DATABASE)) {
+ database = option.getValue();
+ } else if (key.equalsIgnoreCase(CommonConstants.DATABASE)) {
+ // Queries ignore it, as they only read the `database` option: fail
rather than delete from another table
+ throw new IllegalArgumentException(
+ "Unsupported option: " + key + ", set the database with the '" +
CommonConstants.DATABASE + "' option");
+ } else {
+ options.put(key, option.getValue());
+ }
+ }
+ return new DeleteStatement(tableName, predicate, database, options);
+ }
+
+ private static String getTableName(SqlNode targetTable) {
+ // Table hints and EXTEND clauses parse into other node types
+ checkArgument(targetTable instanceof SqlIdentifier, "DELETE only supports
a plain table name, got: %s",
+ targetTable);
+ // A quoted name part may contain a dot, which splits it like the table
name of a query
+ String tableName = String.join(".", ((SqlIdentifier) targetTable).names);
+ // Empty parts, e.g. in "db."."t", are rejected rather than dropped, so
that the table name is the one authorized
+ String[] parts = tableName.split("\\.", -1);
+ checkArgument(parts.length <= 2 &&
Arrays.stream(parts).noneMatch(String::isEmpty),
+ "Invalid table name: %s, expected [database.]table", tableName);
+ return tableName;
+ }
+
+ /// Serializes a WHERE clause back into SQL, and verifies that Pinot parses
it into the same expression.
+ private static String toPinotSql(SqlNode condition) {
+ Expression expression;
+ Expression serializedExpression;
+ String predicate;
+ try {
+ expression = CalciteSqlParser.compileToExpression(condition);
+ predicate = condition.accept(KEYWORD_IDENTIFIER_QUOTER)
+ .toSqlString(config -> config.withDialect(PINOT_SQL_DIALECT)
+ .withQuoteAllIdentifiers(false)
+ .withIndentation(0))
+ .getSql();
+ serializedExpression = CalciteSqlParser.compileToExpression(predicate);
+ } catch (Exception e) {
+ throw new IllegalArgumentException("Unsupported WHERE clause in DELETE:
" + condition, e);
+ }
+ // Fail rather than hand over a predicate that selects other rows than the
statement
+ checkArgument(serializedExpression.equals(expression),
+ "Unsupported WHERE clause in DELETE, it cannot be serialized back into
the same expression: %s", condition);
+ checkNoCrossTableFunction(expression);
+ return predicate;
+ }
+
+ /// Rejects the functions that read another table: the caller is only
authorized for the table it deletes from.
+ private static void checkNoCrossTableFunction(Expression expression) {
+ if (expression.getType() != ExpressionType.FUNCTION) {
+ return;
+ }
+ Function function = expression.getFunctionCall();
+ String functionName = function.getOperator().replace("_",
"").toLowerCase(Locale.ROOT);
+ checkArgument(!CROSS_TABLE_FUNCTIONS.contains(functionName),
+ "Unsupported WHERE clause in DELETE, %s reads another table",
function.getOperator());
+ if (function.getOperands() != null) {
+ for (Expression operand : function.getOperands()) {
+ checkNoCrossTableFunction(operand);
+ }
+ }
+ }
+
+ /// Returns the statement with its table name resolved: qualified with the
database of the request, and in the case
+ /// the table is defined with (table names are case-insensitive by default).
The broker and the controller authorize
+ /// the caller for the resolved table name, and hand the resolved statement
to the executor.
+ ///
+ /// The database of the request is the `database` request header, else the
`database` option of the statement, as
+ /// for a multi-stage query (see
`DatabaseUtils#extractDatabaseFromQueryRequest`). They must match when both are
set,
+ /// and the database of a `database.table` name must match them.
+ ///
+ /// @param databaseHeader value of the `database` request header, if any
+ /// @param tableCache tables of the cluster, to resolve the case of the
table name. A table name it does not know
+ /// keeps the case of the statement.
+ /// @throws QueryException with [QueryErrorCode#QUERY_VALIDATION] if the
`database` header, the `database` option
+ /// and the database of a `database.table` name do
not match (a
+ /// `DatabaseConflictException`), or if the table is
a logical table, which `DELETE` does
+ /// not support
+ public DeleteStatement resolveTableName(@Nullable String databaseHeader,
TableCache tableCache)
+ throws QueryException {
+ String database =
DatabaseUtils.extractDatabaseFromOptionAndHeader(_database, databaseHeader);
+ String tableName;
+ try {
+ tableName = DatabaseUtils.translateTableName(_tableName, database,
tableCache.isIgnoreCase());
+ } catch (IllegalArgumentException e) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException("Invalid table name in
DELETE: " + e.getMessage(), e);
+ }
+ String actualTableName = tableCache.getActualTableName(tableName);
+ if (actualTableName == null &&
tableCache.getActualLogicalTableName(tableName) != null) {
+ // Deleting from a logical table would delete from physical tables the
caller is not authorized for
+ throw QueryErrorCode.QUERY_VALIDATION.asException("DELETE does not
support logical tables: " + tableName);
+ }
+ return new DeleteStatement(actualTableName != null ? actualTableName :
tableName, _predicate, null, _options, true);
Review Comment:
`resolveTableName` now records whether the cache knew the table, exposed as
`tableExists()`, and both the broker and the controller check it after
authorization and fail with `TABLE_DOES_NOT_EXIST` before calling the executor
(d5065b2950). Tests on both roles cover an authorized DELETE on an unknown
table (TABLE_DOES_NOT_EXIST, executor not called) and an unauthorized one
(still 403 / ACCESS_DENIED, so existence isn't leaked), and the integration
test checks it end to end.
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/dml/DeleteStatement.java:
##########
@@ -0,0 +1,307 @@
+/**
+ * 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.dml;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.calcite.avatica.util.Casing;
+import org.apache.calcite.sql.SqlDelete;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlSyntax;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.util.SqlShuttle;
+import org.apache.calcite.sql.validate.SqlValidatorUtil;
+import org.apache.pinot.common.config.provider.TableCache;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DatabaseUtils;
+import org.apache.pinot.spi.config.task.AdhocTaskConfig;
+import org.apache.pinot.spi.exception.QueryErrorCode;
+import org.apache.pinot.spi.exception.QueryException;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.apache.pinot.sql.parsers.SqlNodeAndOptions;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+
+/// A SQL `DELETE FROM <table> WHERE <predicate>` statement.
+///
+/// Pinot parses `DELETE` but does not delete rows itself: the default SQL
executor answers that it is not supported,
+/// and a deployment that can delete rows, e.g. by purging the matching rows
from the segments with a minion task,
+/// executes the parsed statement by overriding
`SqlQueryExecutor#executeDelete`. The generic [#execute()] and
+/// [#generateAdhocTaskConfig()] do not apply to it.
+///
+/// Before handing the statement to the executor, the broker and the
controller resolve its table with
+/// [#resolveTableName] and authorize the caller to delete rows from that
table.
+///
+/// Its options are the `SET` statements, the legacy `OPTION(...)` suffix and
the request `queryOptions` of the
+/// statement (`SET` takes precedence). The `database` option only qualifies
the table name, see [#resolveTableName].
+/// Every other option, including query options such as `timeoutMs`, reaches
the executor as written through
+/// [#getOptions()], so that an option the executor relies on (e.g. a dry run)
cannot be reclassified as a query option
+/// and silently dropped.
+///
+/// Instances are immutable and thread-safe.
+public class DeleteStatement implements DataManipulationStatement {
+ public static final String NOT_SUPPORTED_MESSAGE =
+ "DELETE is not supported by this Pinot cluster, it requires a SQL
executor that implements row deletion";
+
+ /// Serializes the WHERE clause into SQL that the Pinot parser reads back
into the same expression. Combined with
+ /// `quoteAllIdentifiers = false`, identifiers are only quoted (with double
quotes) when they were quoted.
+ private static final SqlDialect PINOT_SQL_DIALECT = new
SqlDialect(SqlDialect.EMPTY_CONTEXT
+ .withIdentifierQuoteString("\"")
+ .withLiteralQuoteString("'")
+ .withLiteralEscapedQuoteString("''")
+ .withUnquotedCasing(Casing.UNCHANGED)
+ .withQuotedCasing(Casing.UNCHANGED)
+ .withCaseSensitive(true));
+
+ /// Quotes the unquoted identifiers named like a SQL function without
arguments, e.g. `user`, `pi` or `current_date`:
+ /// Calcite unparses them as upper-cased keywords, while Pinot reads them as
columns, so they would name another
+ /// column. Mirrors the check of `SqlUtil.unparseSqlIdentifierSyntax`.
+ private static final SqlShuttle KEYWORD_IDENTIFIER_QUOTER = new SqlShuttle()
{
+ @Override
+ public SqlNode visit(SqlIdentifier identifier) {
+ if (identifier.isSimple() && !identifier.getParserPosition().isQuoted())
{
+ SqlOperator operator =
+
SqlValidatorUtil.lookupSqlFunctionByID(SqlStdOperatorTable.instance(),
identifier, null);
+ if (operator != null && (operator.getSyntax() == SqlSyntax.FUNCTION_ID
+ || operator.getSyntax() == SqlSyntax.FUNCTION_ID_CONSTANT)) {
+ return new SqlIdentifier(identifier.names, null,
SqlParserPos.QUOTED_ZERO,
+ List.of(SqlParserPos.QUOTED_ZERO));
+ }
+ }
+ return identifier;
+ }
+ };
+
+ /// Canonical names (lower case, without underscores) of the functions that
read another table than the one the
+ /// statement deletes from, which the caller is not authorized to read.
+ private static final Set<String> CROSS_TABLE_FUNCTIONS = Set.of("lookup",
"insubquery", "inpartitionedsubquery");
+
+ private final String _tableName;
+ private final String _predicate;
+ @Nullable
+ private final String _database;
+ private final Map<String, String> _options;
+ private final boolean _resolved;
+
+ private DeleteStatement(String tableName, String predicate, @Nullable String
database, Map<String, String> options) {
+ this(tableName, predicate, database, options, false);
+ }
+
+ private DeleteStatement(String tableName, String predicate, @Nullable String
database, Map<String, String> options,
+ boolean resolved) {
+ _tableName = tableName;
+ _predicate = predicate;
+ _database = database;
+ _options = Collections.unmodifiableMap(new HashMap<>(options));
+ _resolved = resolved;
+ }
+
+ /// Parses a `DELETE` statement.
+ ///
+ /// @throws IllegalArgumentException if the statement is not a supported
`DELETE`: it must have a WHERE clause, no
+ /// table alias, a WHERE clause that Pinot
can parse as an expression and that does
+ /// not read another table (e.g. with
`lookUp` or `IN_SUBQUERY`), and set the
+ /// database with the `database` option only
+ public static DeleteStatement parse(SqlNodeAndOptions sqlNodeAndOptions) {
+ SqlNode sqlNode = sqlNodeAndOptions.getSqlNode();
+ checkArgument(sqlNode instanceof SqlDelete, "Not a DELETE statement: %s",
sqlNode.getKind());
+ SqlDelete sqlDelete = (SqlDelete) sqlNode;
+ String tableName = getTableName(sqlDelete.getTargetTable());
+ checkArgument(sqlDelete.getAlias() == null,
+ "DELETE does not support a table alias, reference the columns
directly");
+ SqlNode condition = sqlDelete.getCondition();
+ checkArgument(condition != null,
+ "DELETE requires a WHERE clause; delete the table segments to remove
all of its rows");
+ String predicate = toPinotSql(condition);
+
+ String database = null;
+ Map<String, String> options = new HashMap<>();
+ for (Map.Entry<String, String> option :
sqlNodeAndOptions.getOptions().entrySet()) {
+ String key = option.getKey();
+ if (key.equals(CommonConstants.DATABASE)) {
+ database = option.getValue();
+ } else if (key.equalsIgnoreCase(CommonConstants.DATABASE)) {
+ // Queries ignore it, as they only read the `database` option: fail
rather than delete from another table
+ throw new IllegalArgumentException(
+ "Unsupported option: " + key + ", set the database with the '" +
CommonConstants.DATABASE + "' option");
+ } else {
+ options.put(key, option.getValue());
+ }
+ }
+ return new DeleteStatement(tableName, predicate, database, options);
+ }
+
+ private static String getTableName(SqlNode targetTable) {
+ // Table hints and EXTEND clauses parse into other node types
+ checkArgument(targetTable instanceof SqlIdentifier, "DELETE only supports
a plain table name, got: %s",
+ targetTable);
+ // A quoted name part may contain a dot, which splits it like the table
name of a query
+ String tableName = String.join(".", ((SqlIdentifier) targetTable).names);
+ // Empty parts, e.g. in "db."."t", are rejected rather than dropped, so
that the table name is the one authorized
+ String[] parts = tableName.split("\\.", -1);
+ checkArgument(parts.length <= 2 &&
Arrays.stream(parts).noneMatch(String::isEmpty),
+ "Invalid table name: %s, expected [database.]table", tableName);
+ return tableName;
+ }
+
+ /// Serializes a WHERE clause back into SQL, and verifies that Pinot parses
it into the same expression.
+ private static String toPinotSql(SqlNode condition) {
+ Expression expression;
+ Expression serializedExpression;
+ String predicate;
+ try {
+ expression = CalciteSqlParser.compileToExpression(condition);
+ predicate = condition.accept(KEYWORD_IDENTIFIER_QUOTER)
+ .toSqlString(config -> config.withDialect(PINOT_SQL_DIALECT)
+ .withQuoteAllIdentifiers(false)
+ .withIndentation(0))
+ .getSql();
+ serializedExpression = CalciteSqlParser.compileToExpression(predicate);
+ } catch (Exception e) {
+ throw new IllegalArgumentException("Unsupported WHERE clause in DELETE:
" + condition, e);
Review Comment:
Fixed in d5065b2950: the catch no longer re-renders the node blindly; a
small `describe()` helper returns `toString()` or a placeholder when rendering
throws, so `AT TIME ZONE` now surfaces as `IllegalArgumentException` and
SQL_PARSING on both roles, with the cause carrying the detail. I used the
helper rather than `Strings.lenientFormat` because lenientFormat logs a WARNING
with the full stack trace every time `toString` throws, which is exactly the
noise we wanted gone. Test added.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -397,6 +405,10 @@ private StreamingOutput executeSqlQuery(@Context
HttpHeaders httpHeaders, String
throw QueryErrorCode.QUERY_VALIDATION.asException(
"DDL statements are not supported on /sql; use POST /sql/ddl
instead.");
}
+ if (isGet && sqlNodeAndOptions.getSqlNode() instanceof SqlDelete) {
Review Comment:
GET `/sql` now rejects every non-DQL statement with SQL_PARSING like the
broker does (d5065b2950); the DELETE test moved to that code and a GET `INSERT
INTO ... FROM FILE` case asserts `executeDMLStatement` is never called.
--
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]
