xiangfu0 commented on code in PR #19664: URL: https://github.com/apache/pinot/pull/19664#discussion_r4237851274
########## pinot-common/src/main/java/org/apache/pinot/sql/parsers/dml/DeleteStatement.java: ########## @@ -0,0 +1,351 @@ +/** + * 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`. Its execution type is +/// [ExecutionType#EXECUTOR]: the generic [#execute()], [#getResultSchema()] and [#generateAdhocTaskConfig()] do not +/// apply to it. +/// +/// Before handing the statement to the executor, the broker and the controller resolve its table with +/// [#resolveTableName], authorize the caller to delete rows from that table, and check that the table exists with +/// [#tableExists()]. +/// +/// 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`, is passed to the executor 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 endpoint, 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"); + + /// Canonical names of the functions that run a script. Rejected because the broker's groovy policy + /// (`pinot.broker.disable.query.groovy`, the per-table `queryConfig.disableGroovy` override and the static + /// analyzer) only applies to queries, not to a `DELETE`, and the executor may run the predicate on another role. + private static final Set<String> SCRIPT_FUNCTIONS = Set.of("groovy"); + + private final String _tableName; + private final String _predicate; + @Nullable + private final String _database; + private final Map<String, String> _options; + private final boolean _resolved; + private final boolean _tableExists; + + private DeleteStatement(String tableName, String predicate, @Nullable String database, Map<String, String> options) { + this(tableName, predicate, database, options, false, false); + } + + private DeleteStatement(String tableName, String predicate, @Nullable String database, Map<String, String> options, + boolean resolved, boolean tableExists) { + _tableName = tableName; + _predicate = predicate; + _database = database; + _options = Collections.unmodifiableMap(new HashMap<>(options)); + _resolved = resolved; + _tableExists = tableExists; + } + + /// 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, that does + /// not read another table (e.g. with `lookUp` or `IN_SUBQUERY`) and that does not + /// run a script (`groovy`), 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 to prevent an accidental full-table delete; use " + + "an explicit always-true predicate or delete the table segments to remove all 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 only read the exact `database` option (where they read it at all, see resolveTableName): 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: " + describe(condition), e); + } + // Fail rather than hand over a predicate that selects other rows than the statement + checkArgument(serializedExpression.equals(expression), Review Comment: Fixed in b5d0ecb260: a visitor over the parsed WHERE node now rejects, before the clause is compiled, interval literals (quoted and unquoted `INTERVAL 30 DAY` forms), `BETWEEN SYMMETRIC`, exact integer literals outside the long range, and, while re-verifying the same family, decimal literals that a double does not represent exactly, literals outside the double range, and `LIKE ... ESCAPE`, whose escape character the single-stage evaluator drops. Each has its own message, and `DeleteStatementTest` covers the rejected and the still-accepted shapes (`TIMESTAMPADD`, `ASYMMETRIC`, `Long.MAX_VALUE`, `1.5E19`, plain `LIKE`). _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ ########## pinot-common/src/main/java/org/apache/pinot/sql/parsers/dml/DeleteStatement.java: ########## @@ -0,0 +1,351 @@ +/** + * 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`. Its execution type is +/// [ExecutionType#EXECUTOR]: the generic [#execute()], [#getResultSchema()] and [#generateAdhocTaskConfig()] do not +/// apply to it. +/// +/// Before handing the statement to the executor, the broker and the controller resolve its table with +/// [#resolveTableName], authorize the caller to delete rows from that table, and check that the table exists with +/// [#tableExists()]. +/// +/// 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`, is passed to the executor 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 endpoint, 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"); + + /// Canonical names of the functions that run a script. Rejected because the broker's groovy policy + /// (`pinot.broker.disable.query.groovy`, the per-table `queryConfig.disableGroovy` override and the static + /// analyzer) only applies to queries, not to a `DELETE`, and the executor may run the predicate on another role. + private static final Set<String> SCRIPT_FUNCTIONS = Set.of("groovy"); + + private final String _tableName; + private final String _predicate; + @Nullable + private final String _database; + private final Map<String, String> _options; + private final boolean _resolved; + private final boolean _tableExists; + + private DeleteStatement(String tableName, String predicate, @Nullable String database, Map<String, String> options) { + this(tableName, predicate, database, options, false, false); + } + + private DeleteStatement(String tableName, String predicate, @Nullable String database, Map<String, String> options, + boolean resolved, boolean tableExists) { + _tableName = tableName; + _predicate = predicate; + _database = database; + _options = Collections.unmodifiableMap(new HashMap<>(options)); + _resolved = resolved; + _tableExists = tableExists; + } + + /// 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, that does + /// not read another table (e.g. with `lookUp` or `IN_SUBQUERY`) and that does not + /// run a script (`groovy`), 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 to prevent an accidental full-table delete; use " + + "an explicit always-true predicate or delete the table segments to remove all 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 only read the exact `database` option (where they read it at all, see resolveTableName): 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: " + describe(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); + checkFunctions(expression); + return predicate; + } + + /// Renders a node for an error message without failing: rendering it is what may have failed, e.g. for an operator + /// without unparse support such as `AT TIME ZONE`, in which case the cause of the error carries the detail. + private static String describe(SqlNode node) { + try { + return node.toString(); + } catch (RuntimeException e) { + return "<" + node.getKind() + " expression that cannot be rendered>"; + } + } + + /// Rejects the functions that read another table, as the caller is only authorized for the table it deletes from, + /// and the functions that run a script, see [#SCRIPT_FUNCTIONS]. + private static void checkFunctions(Expression expression) { + if (expression.getType() != ExpressionType.FUNCTION) { + return; + } + Function function = expression.getFunctionCall(); + String functionName = function.getOperator().replace("_", "").toLowerCase(Locale.ROOT); Review Comment: Done in b5d0ecb260: `checkFunctions` folds with `toUpperCase(Locale.ROOT)` against upper-case deny-lists, so the dotless `ı` maps back to `I`, and any operator name with a non-ASCII character is rejected outright since no supported function has one. Tests run the `IN_SUBQUERY`/`IN_PARTITIONED_SUBQUERY`/`lookUp`/`groovy` cases under `tr-TR` (default locale restored in a finally) and with the U+0131 spellings typed directly. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ ########## pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java: ########## @@ -713,6 +739,137 @@ 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, followed by the table check without a table, so that + /// an access control that only authenticates the caller in that check rejects an unauthenticated caller before the + /// table is looked up. 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. A table the table cache does not know + /// fails with [QueryErrorCode#TABLE_DOES_NOT_EXIST], as a query does, but only once the caller is authorized for + /// it, so that the existence of a table is not leaked to a caller who is not authorized for it. + 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()) { + // Access controls that authenticate the caller in their table check rather than in the first step (e.g. the + // ZooKeeper basic auth) reject an unauthenticated caller here, before the table is looked up + authorizationResult = accessControl.authorize(requesterIdentity, Set.of()); + } + 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); + if (!statement.tableExists()) { + return new BrokerResponseNative(QueryErrorCode.TABLE_DOES_NOT_EXIST, + "Table does not exist: " + statement.getTableName()); + } + LOGGER.info("Executing DELETE on table: {}, predicate: {}, options: {}, client: {}", statement.getTableName(), Review Comment: Changed in b5d0ecb260: the line is built by `deleteLogMessage`, which applies the broker's `sqlRedaction` mode to the predicate through the query log's redaction (with a fingerprint of the statement in `literalValues` mode, so the structure is kept), single-lines the option keys and the client (backslash, CR and LF escaped as the query log does), and only logs the client when `pinot.broker.request.client.ip.logging` is on. Tests cover `full` and `literalValues` redaction and a newline inside a string literal. The controller line now escapes the predicate, option keys and client too (it has no redaction setting, as you noted), with tests. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ ########## pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java: ########## @@ -145,13 +141,21 @@ public static SqlNodeAndOptions compileToSqlNodeAndOptions(String sql) SqlNodeAndOptions sqlNodeAndOptions = extractSqlNodeAndOptions(sqlNodeList); // add legacy OPTIONS keyword-based options if (!options.isEmpty()) { - Map<String, String> optionMap = extractOptionsMap(options); - if (sqlNodeAndOptions.getSqlType() == PinotSqlType.DQL) { - // No-op unless the broker enables query option validation. DML (e.g. - // INSERT INTO FILE OPTION(taskName=...)) carries free-form task/FS properties, like DML SET. - QueryOptionsUtils.validateSqlQueryOptions(optionMap); + if (legacyOptionSyntaxMode == SqlOptionsMode.IGNORE) { + // Dropping a DELETE option (e.g. dryRun) would change its effect. + if (sqlNodeAndOptions.getSqlNode() instanceof SqlDelete) { Review Comment: Agreed and changed in b5d0ecb260: both the legacy `OPTION(...)` check in `CalciteSqlParser` and the `sqlOptionsMode=IGNORE` check in `RequestUtils.setOptions` are keyed on `PinotSqlType.DML`, so an INSERT carrying options fails in IGNORE mode instead of silently losing its task configuration. `SqlOptionsModeTest` now expects the error for INSERT, and the `CommonConstants`/`SqlOptionsMode` Javadoc says DML. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ ########## pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java: ########## @@ -715,6 +714,11 @@ private void setUpPinotController() { // max.segment.completion.time.millis is seeded from cluster config before serving requests. _clusterConfigChangeHandler.registerClusterConfigChangeListener(_pinotLLCRealtimeSegmentManager); LOGGER.info("Registered PinotLLCRealtimeSegmentManager as cluster config change listener"); + // Keeps the query option cluster configs in effect for the statements the controller parses on /sql, as the broker + // does: the legacy OPTION(...) syntax mode for every statement, including the DML it executes itself (a DELETE + // carrying legacy options fails in IGNORE and REJECT mode), and the validation mode for the queries it parses + // before forwarding them + _clusterConfigChangeHandler.registerClusterConfigChangeListener(new QueryOptionConfigListener()); Review Comment: Done in b5d0ecb260: `QueryOptionConfigListener.legacySyntaxModeOnly()` applies only the legacy syntax mode, and that is what the controller registers, with a `QueryOptionConfigListenerTest` case showing the validation mode is left alone. The `parseQuery` call in `validateMultiStageQuery` moved inside the per-query try, so a query that fails to parse (e.g. legacy `OPTION(...)` under REJECT) reports `compiledSuccessfully=false` for that entry instead of failing the request; `PinotQueryResourceTest` pins it. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ ########## pinot-common/src/main/java/org/apache/pinot/sql/parsers/dml/DeleteStatement.java: ########## @@ -0,0 +1,351 @@ +/** + * 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`. Its execution type is +/// [ExecutionType#EXECUTOR]: the generic [#execute()], [#getResultSchema()] and [#generateAdhocTaskConfig()] do not +/// apply to it. +/// +/// Before handing the statement to the executor, the broker and the controller resolve its table with +/// [#resolveTableName], authorize the caller to delete rows from that table, and check that the table exists with +/// [#tableExists()]. +/// +/// 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`, is passed to the executor 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 endpoint, 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"); + + /// Canonical names of the functions that run a script. Rejected because the broker's groovy policy + /// (`pinot.broker.disable.query.groovy`, the per-table `queryConfig.disableGroovy` override and the static + /// analyzer) only applies to queries, not to a `DELETE`, and the executor may run the predicate on another role. + private static final Set<String> SCRIPT_FUNCTIONS = Set.of("groovy"); + + private final String _tableName; + private final String _predicate; + @Nullable + private final String _database; + private final Map<String, String> _options; + private final boolean _resolved; + private final boolean _tableExists; + + private DeleteStatement(String tableName, String predicate, @Nullable String database, Map<String, String> options) { + this(tableName, predicate, database, options, false, false); + } + + private DeleteStatement(String tableName, String predicate, @Nullable String database, Map<String, String> options, + boolean resolved, boolean tableExists) { + _tableName = tableName; + _predicate = predicate; + _database = database; + _options = Collections.unmodifiableMap(new HashMap<>(options)); + _resolved = resolved; + _tableExists = tableExists; + } + + /// 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, that does + /// not read another table (e.g. with `lookUp` or `IN_SUBQUERY`) and that does not + /// run a script (`groovy`), 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 to prevent an accidental full-table delete; use " + + "an explicit always-true predicate or delete the table segments to remove all 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 only read the exact `database` option (where they read it at all, see resolveTableName): 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: " + describe(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); + checkFunctions(expression); + return predicate; + } + + /// Renders a node for an error message without failing: rendering it is what may have failed, e.g. for an operator + /// without unparse support such as `AT TIME ZONE`, in which case the cause of the error carries the detail. + private static String describe(SqlNode node) { + try { + return node.toString(); + } catch (RuntimeException e) { + return "<" + node.getKind() + " expression that cannot be rendered>"; + } + } + + /// Rejects the functions that read another table, as the caller is only authorized for the table it deletes from, + /// and the functions that run a script, see [#SCRIPT_FUNCTIONS]. + private static void checkFunctions(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()); + checkArgument(!SCRIPT_FUNCTIONS.contains(functionName), + "Unsupported WHERE clause in DELETE, %s is not supported in a DELETE predicate", function.getOperator()); + if (function.getOperands() != null) { + for (Expression operand : function.getOperands()) { + checkFunctions(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, check that the table exists with [#tableExists()], 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`) and for the controller `/sql` + /// endpoint. They must match when both are set, and the database of a `database.table` name must match them. Note + /// that the broker's single-stage engine resolves the table of a query from the `database` header only, so a + /// preview of the predicate with `SET database` on that engine may read another table than the one the `DELETE` + /// resolves to: preview with the `database` header or a qualified `database.table` name instead. + /// + /// @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, and the resolved statement reports [#tableExists()] false. + /// @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); Review Comment: Fair point for a destructive statement. In b5d0ecb260 `resolveTableName` rejects the `database` option on an unqualified table name when the `database` header is not set, with a message asking for the header or a `database.table` name; option plus matching header and qualified name plus option still resolve as before. `DeleteStatementTest` and the executor test fixture were updated accordingly. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ -- 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]
