xiangfu0 commented on code in PR #19664: URL: https://github.com/apache/pinot/pull/19664#discussion_r4244660626
########## pinot-common/src/main/java/org/apache/pinot/sql/parsers/dml/DeleteStatement.java: ########## @@ -0,0 +1,480 @@ +/** + * 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.math.BigDecimal; +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.SqlCall; +import org.apache.calcite.sql.SqlDelete; +import org.apache.calcite.sql.SqlDialect; +import org.apache.calcite.sql.SqlIdentifier; +import org.apache.calcite.sql.SqlIntervalLiteral; +import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.SqlLiteral; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.SqlNumericLiteral; +import org.apache.calcite.sql.SqlOperator; +import org.apache.calcite.sql.SqlSyntax; +import org.apache.calcite.sql.SqlWindow; +import org.apache.calcite.sql.fun.SqlBetweenOperator; +import org.apache.calcite.sql.fun.SqlLikeOperator; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.parser.SqlParserPos; +import org.apache.calcite.sql.util.SqlBasicVisitor; +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 is not a +/// logical table with [#isLogicalTable()] and that it 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 (upper 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. Upper case because the compiled function + /// names are lower-cased with the default locale, and lower-casing with a Turkish locale turns `I` into the dotless + /// `ı` (U+0131), which only an upper-case fold with [Locale#ROOT] maps back to `I`. + 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"); + + /// Canonical names of the window function operators. A window function is never valid in a `DELETE` predicate, and + /// the compiled expression hides its window specification (and any cross-table or script function call in it) in a + /// string literal, see [#PREDICATE_NODE_CHECKER]. + private static final Set<String> WINDOW_FUNCTIONS = Set.of("OVER"); + + private static final BigDecimal LONG_MIN_VALUE = BigDecimal.valueOf(Long.MIN_VALUE); + private static final BigDecimal LONG_MAX_VALUE = BigDecimal.valueOf(Long.MAX_VALUE); + + /// Shared by the quoted interval literal (`INTERVAL '30' DAY`, a [SqlIntervalLiteral]) and the unquoted one + /// (`INTERVAL 30 DAY` or `INTERVAL (expr) DAY`, a [SqlCall] of kind [SqlKind#INTERVAL]), which the parser accepts + /// with its BABEL conformance. + private static final String INTERVAL_MESSAGE = "Unsupported WHERE clause in DELETE, interval literals are not " + + "supported in a DELETE predicate, their unit is lost (INTERVAL '30' DAY) or not evaluated (INTERVAL 30 DAY): " + + "use TIMESTAMPADD or a millisecond value"; + + /// Rejects the WHERE clause nodes whose single-stage [Expression] selects other rows than the SQL, so that the + /// round-trip check of [#toPinotSql] cannot catch them, and the window functions, whose window specification is + /// compiled into a string literal that hides the functions it calls from [#checkFunctions]. Runs on the parsed + /// [SqlNode] before it is compiled, so that the statement fails with a clear message. + /// + /// - A quoted interval literal compiles to its bare value: `INTERVAL '30' DAY` becomes the literal `'30'`. An + /// unquoted one, `INTERVAL 30 DAY`, compiles to a call of a function `interval` that no engine evaluates. + /// - `BETWEEN SYMMETRIC` compiles to a plain `BETWEEN`, which never matches an inverted range. + /// - `LIKE ... ESCAPE` compiles to a three-operand `LIKE` whose escape character the single-stage filter ignores: + /// the escaped wildcard still matches anything, and the escape character is matched literally. + /// - An exact integer literal outside the long range wraps around when it is compiled. + /// - Every other numeric literal compiles to a double: a decimal literal that a double does not represent exactly + /// (more than about 16 significant digits, e.g. `12345678901234567.89`) is rounded, which moves the bound a + /// `BIG_DECIMAL` column is compared against, and a literal outside the double range (`1E999`, `1E-999`) + /// becomes infinite or zero. The exact value is kept with `CAST('<digits>' AS DECIMAL)`, which compiles to a + /// `BigDecimal` literal. + private static final SqlBasicVisitor<Void> PREDICATE_NODE_CHECKER = new SqlBasicVisitor<>() { + @Override + public Void visit(SqlLiteral literal) { + checkArgument(!(literal instanceof SqlIntervalLiteral), INTERVAL_MESSAGE); + if (literal instanceof SqlNumericLiteral) { + SqlNumericLiteral numericLiteral = (SqlNumericLiteral) literal; + BigDecimal value = numericLiteral.bigDecimalValue(); + checkArgument(value != null, "Unsupported WHERE clause in DELETE, invalid numeric literal: %s", + numericLiteral.toValue()); + if (numericLiteral.isExact() && numericLiteral.isInteger()) { + checkArgument(value.compareTo(LONG_MIN_VALUE) >= 0 && value.compareTo(LONG_MAX_VALUE) <= 0, + "Unsupported WHERE clause in DELETE, integer literal out of the long range: %s", + numericLiteral.toValue()); + } else { + // Compiled as a double (RequestUtils.getLiteral): reject the overflow to an infinity and the underflow to 0 + double doubleValue = value.doubleValue(); + checkArgument(!Double.isInfinite(doubleValue) && (doubleValue != 0 || value.signum() == 0), + "Unsupported WHERE clause in DELETE, numeric literal out of the double range: %s", + numericLiteral.toValue()); + if (numericLiteral.isExact()) { + // BigDecimal.valueOf(double) is new BigDecimal(Double.toString(double)), the bound a BIG_DECIMAL column is + // compared against: the literal is accepted iff it survives the round trip through a double + checkArgument(BigDecimal.valueOf(doubleValue).compareTo(value) == 0, + "Unsupported WHERE clause in DELETE, decimal literal %s loses precision as a double: " + + "use CAST('%s' AS DECIMAL)", numericLiteral.toValue(), numericLiteral.toValue()); + } + } + } + return null; + } + + @Override + public Void visit(SqlCall call) { + checkArgument(call.getKind() != SqlKind.INTERVAL, INTERVAL_MESSAGE); + checkArgument(call.getKind() != SqlKind.OVER && !(call instanceof SqlWindow), + "Unsupported WHERE clause in DELETE, window functions are not supported in a DELETE predicate"); + if (call.getOperator() instanceof SqlBetweenOperator) { + checkArgument(((SqlBetweenOperator) call.getOperator()).flag != SqlBetweenOperator.Flag.SYMMETRIC, + "Unsupported WHERE clause in DELETE, BETWEEN SYMMETRIC is not supported in a DELETE predicate"); + } + // Also NOT LIKE, ILIKE and SIMILAR TO, which share the operator class + if (call.getOperator() instanceof SqlLikeOperator) { + checkArgument(call.operandCount() != 3, "Unsupported WHERE clause in DELETE, LIKE ... ESCAPE is not supported " + + "in a DELETE predicate, the escape character is ignored by the single-stage engine"); + } + return super.visit(call); + } + }; + + 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 final boolean _logicalTable; + + private DeleteStatement(String tableName, String predicate, @Nullable String database, Map<String, String> options) { + this(tableName, predicate, database, options, false, false, false); + } + + private DeleteStatement(String tableName, String predicate, @Nullable String database, Map<String, String> options, + boolean resolved, boolean tableExists, boolean logicalTable) { + _tableName = tableName; + _predicate = predicate; + _database = database; + _options = Collections.unmodifiableMap(new HashMap<>(options)); + _resolved = resolved; + _tableExists = tableExists; + _logicalTable = logicalTable; + } + + /// 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`), that does not + /// run a script (`groovy`), that uses no window function, interval literal + /// (quoted or not), `BETWEEN SYMMETRIC`, `LIKE ... ESCAPE`, integer literal + /// outside the long range, decimal literal that a double does not represent + /// exactly or numeric literal outside the double range (see + /// [#PREDICATE_NODE_CHECKER]), 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) { + // Before compiling, as the compiled expression does not tell these nodes apart from supported ones + condition.accept(PREDICATE_NODE_CHECKER); + 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); Review Comment: **[Cleanup — efficiency] The verified `Expression` is discarded, forcing executors to compile the predicate a third time.** `toPinotSql` compiles the condition, re-parses+re-compiles the serialized form for the round-trip check, then throws both `Expression`s away — `DeleteStatement` keeps only the string, so every executor re-runs `CalciteSqlParser.compileToExpression(getPredicate())`. Keeping the verified expression as a field (e.g. `getPredicateExpression()`) gives executors the exact object the round-trip validated, for free. ########## pinot-core/src/main/java/org/apache/pinot/core/query/executor/sql/SqlQueryExecutor.java: ########## @@ -112,11 +187,45 @@ public BrokerResponse executeDMLStatement(SqlNodeAndOptions sqlNodeAndOptions, return result; } + /// Executes a `DELETE` statement. Pinot does not delete rows itself, so this implementation answers with a + /// [QueryErrorCode#QUERY_VALIDATION] error: executors that implement row deletion override it. + /// + /// The broker and the controller each create their executor through their own hook, + /// `BaseBrokerStarter#createSqlQueryExecutor` and `BaseControllerStarter#createSqlQueryExecutor`: both must be + /// overridden for a `DELETE` to behave the same on the two SQL endpoints, since the role that receives the statement + /// executes it with its own executor. + /// + /// The query endpoints of the broker and the controller (`PinotClientRequest`, `PinotQueryResource`) resolve the + /// table of the statement with [DeleteStatement#resolveTableName] and authorize the caller to delete rows from it + /// before calling it (through [#executeStatement]): implementations delete rows from that exact table, + /// [DeleteStatement#getTableName()]. Neither applies quotas nor logs the statement as a query. [#executeStatement] + /// only checks that the statement is resolved, see [DeleteStatement#isResolved()], not that the caller is + /// authorized: an entry point other than those endpoints must authorize the caller itself. + /// + /// Implementations validate the predicate (see [DeleteStatement#getPredicate()]) and the options they read (see + /// [DeleteStatement#getOptions()]) before deleting rows, and forward the request headers to the APIs they call, which + /// authorize the caller again. They report a validation or execution failure either in the response they return or + /// by throwing a [QueryException] with the [QueryErrorCode] to surface, which [#executeStatement] returns as the + /// error of the response; any other exception is logged with its stack trace and returned as a + /// [QueryErrorCode#QUERY_EXECUTION] error. + /// + /// @param statement parsed statement, with its table resolved + /// @param headers headers of the original request, e.g. to authorize the caller + /// @return the response of the statement + /// @throws QueryException to report a failure with its error code, see above + protected BrokerResponse executeDelete(DeleteStatement statement, @Nullable Map<String, String> headers) { + return new BrokerResponseNative(QueryErrorCode.QUERY_VALIDATION, DeleteStatement.NOT_SUPPORTED_MESSAGE); + } + + /// Message of an exception for the error of a response: its message, or, when it has none, its string form (its + /// class name), as the response constructor would otherwise print `null`. + private static String errorMessage(Exception e) { Review Comment: **[Cleanup — reuse] `errorMessage(Exception)` re-implements existing helpers.** `org.apache.pinot.common.utils.ExceptionUtils.consolidateExceptionMessages` (already on this module's classpath) additionally walks the cause chain and skips blank messages — a `QueryException` wrapping the real cause would surface a more useful error; commons-lang3 `ExceptionUtils.getMessage(e)` gives the same class-name fallback as this helper. Either beats a new private method. ########## pinot-common/src/test/java/org/apache/pinot/sql/parsers/dml/DeleteStatementTest.java: ########## @@ -0,0 +1,528 @@ +/** + * 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.List; +import java.util.Locale; +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.calcite.sql.SqlCall; +import org.apache.calcite.sql.SqlDelete; +import org.apache.calcite.sql.SqlFunction; +import org.apache.calcite.sql.SqlFunctionCategory; +import org.apache.calcite.sql.SqlIdentifier; +import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.SqlWriter; +import org.apache.calcite.sql.parser.SqlParserPos; +import org.apache.calcite.sql.type.OperandTypes; +import org.apache.calcite.sql.type.ReturnTypes; +import org.apache.pinot.common.config.provider.TableCache; +import org.apache.pinot.common.utils.request.RequestUtils; +import org.apache.pinot.spi.exception.DatabaseConflictException; +import org.apache.pinot.spi.exception.QueryErrorCode; +import org.apache.pinot.spi.exception.QueryException; +import org.apache.pinot.spi.utils.CommonConstants.Broker.Request; +import org.apache.pinot.spi.utils.JsonUtils; +import org.apache.pinot.sql.parsers.CalciteSqlParser; +import org.apache.pinot.sql.parsers.PinotSqlType; +import org.apache.pinot.sql.parsers.SqlNodeAndOptions; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertTrue; +import static org.testng.Assert.expectThrows; + + +public class DeleteStatementTest { Review Comment: **[Convention — CLAUDE.md] Three new test classes lack class-level Javadoc.** Repo CLAUDE.md: "Add class-level Javadoc for new classes; describe behavior and thread-safety." `DeleteStatementTest`, `SqlQueryExecutorTest` and `BrokerRequestHandlerDelegateTest` are new and have none, while the PR's other new classes (`DeleteStatement`, `ZkBasicAuthAccessControlFactoryTest`) do — a one-liner each would close the gap. -- 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]
