xiangfu0 commented on code in PR #19664: URL: https://github.com/apache/pinot/pull/19664#discussion_r4244655214
########## 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) { Review Comment: **[CONFIRMED — correctness/security] Function-call `like(col, pattern, escape)` bypasses the `LIKE ... ESCAPE` rejection.** This guard only matches the operator form (`call.getOperator() instanceof SqlLikeOperator`). Written as a function call, `DELETE FROM t WHERE like(col, 'a!%b', '!')` parses as a `SqlBasicCall` with `SqlUnresolvedFunction` (kind `OTHER_FUNCTION`, 3 operands), passes `PREDICATE_NODE_CHECKER` and the round-trip check, and reaches the executor. The single-stage engine then builds the LIKE filter from operands 0 and 1 only (`RequestContextUtils`), silently dropping the escape operand — `!%` matches anything instead of a literal `%`, so the DELETE selects (and irreversibly deletes) more rows than the SQL reads. Verified with a runtime harness against this head: the function form is accepted while the operator form is rejected. Suggested fix: in `checkFunctions`, reject the canonical name `LIKE` (and `NOTLIKE`/`SIMILARTO`) when the compiled function has 3 operands, mirroring the node-level check. -- 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]
