xiangfu0 commented on code in PR #19664:
URL: https://github.com/apache/pinot/pull/19664#discussion_r4276083069
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -122,10 +123,23 @@ public static SqlNodeAndOptions
compileToSqlNodeAndOptions(String sql)
sql = ParserUtils.sanitizeSql(sql);
// extract and remove OPTIONS string
- List<String> options = List.of();
SqlOptionsMode legacyOptionSyntaxMode =
QueryOptionsUtils.getLegacyOptionSyntaxMode();
+ List<String> options = List.of();
+ // The suffix text dropped in IGNORE mode, kept only to fail a DELETE (its
options configure it)
+ String ignoredOptions = null;
if (legacyOptionSyntaxMode == SqlOptionsMode.IGNORE) {
- sql = removeOptionsFromSql(sql);
+ // IGNORE differs from the other modes: the options are never applied,
so a single regex pass both detects and
+ // strips the suffix, and the common no-suffix path pays for one scan
without materializing an option list
+ Matcher matcher = OPTIONS_REGEX_PATTEN.matcher(sql);
+ if (matcher.find()) {
+ ignoredOptions = matcher.group(1);
+ StringBuilder strippedSql = new StringBuilder(sql.length());
Review Comment:
Done in a8931cfa73: the suffix is anchored to the end of the statement, so
the strip is now `sql.substring(0, matcher.start())` with no builder or
replacement loop; only the group offsets are kept, and the option text is
materialized from the original input solely when a DELETE is rejected.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -713,6 +749,250 @@ 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 logical table fails with
[QueryErrorCode#QUERY_VALIDATION], and a table the table cache
+ /// does not know with [QueryErrorCode#TABLE_DOES_NOT_EXIST], as a query
does, but only once the caller is
+ /// authorized for it, and every message names the table as the caller wrote
it (see [#deleteAccessDenied] for the
+ /// reason of a denial), so that neither the existence of a table nor its
name as defined is 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()) {
+ throw deleteAccessDenied(null, null, authorizationResult);
+ }
+ // 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 = authorizeTables(accessControl, requesterIdentity,
Set.of());
+ if (!authorizationResult.hasAccess()) {
+ throw deleteAccessDenied(null, null, authorizationResult);
+ }
+ if (_tableCache == null) {
+ return new BrokerResponseNative(QueryErrorCode.QUERY_VALIDATION,
+ "DELETE is not supported by this broker: no table cache was
configured");
+ }
+ DeleteStatement parsed;
+ DeleteStatement statement;
+ try {
+ String databaseHeader = httpHeaders != null ?
httpHeaders.getHeaderString(CommonConstants.DATABASE) : null;
+ parsed = (DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ statement = parsed.resolveTableName(databaseHeader, _tableCache);
+ } catch (QueryException e) {
+ // e.g. an invalid statement, or a database header that does not match
the statement
+ return new BrokerResponseNative(e.getErrorCode(), e.getMessage());
+ }
+ // The caller is authorized for the table as resolved, but the messages
name it as written
+ String writtenTableName = parsed.getTableName();
+ authorizeDelete(accessControl, statement.getTableName(), writtenTableName,
requesterIdentity, httpHeaders);
+ if (statement.isLogicalTable()) {
+ return new BrokerResponseNative(QueryErrorCode.QUERY_VALIDATION,
+ "DELETE does not support logical tables: " + writtenTableName);
+ }
+ if (!statement.tableExists()) {
+ return new BrokerResponseNative(QueryErrorCode.TABLE_DOES_NOT_EXIST,
+ "Table does not exist: " + writtenTableName);
+ }
+ LOGGER.info(deleteLogMessage(statement, sqlNodeAndOptions,
requesterIdentity));
Review Comment:
Done in a8931cfa73: both `executeDelete` sites now guard the call with
`LOGGER.isInfoEnabled()`, so the line is formatted (and the statement
fingerprinted) only when it will be logged.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-core/src/main/java/org/apache/pinot/core/query/executor/sql/SqlQueryExecutor.java:
##########
@@ -76,16 +94,74 @@ private static String getControllerBaseUrl(HelixManager
helixManager) {
return controllerBaseUrl;
}
- /// Execute DML Statement
+ /// Parses and executes a DML statement.
+ ///
+ /// A `DELETE` is refused by [#executeStatement] with a
[QueryErrorCode#ACCESS_DENIED] error, since the statement
+ /// it parses is not resolved: a `DELETE` comes through the query endpoint
of a broker or a controller, which resolves
+ /// its table, authorizes the caller to delete rows from it and then hands
the resolved statement to
+ /// [#executeStatement].
///
/// @param sqlNodeAndOptions Parsed DML object
/// @param headers extra headers map for minion task submission
/// @return BrokerResponse is the DML executed response
public BrokerResponse executeDMLStatement(SqlNodeAndOptions
sqlNodeAndOptions,
@Nullable Map<String, String> headers) {
- DataManipulationStatement statement =
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ DataManipulationStatement statement;
+ try {
+ statement = DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ } catch (QueryException e) {
+ // e.g. a DELETE without a WHERE clause
+ return new BrokerResponseNative(e.getErrorCode(), e.getMessage());
+ }
+ return executeStatement(statement, headers);
+ }
+
+ /// Executes a parsed DML statement, e.g. from
[DataManipulationStatementParser#parse], as its
+ /// [DataManipulationStatement#getExecutionType()] tells: in-process, as a
minion task, or through the hook of this
+ /// executor for the statement
([DataManipulationStatement.ExecutionType#EXECUTOR]), e.g. [#executeDelete] for
a
+ /// [DeleteStatement].
+ ///
+ /// It does not authorize the caller: the query endpoints of the broker and
the controller resolve the table of a
+ /// [DeleteStatement] with [DeleteStatement#resolveTableName] and authorize
the caller to delete rows from it before
+ /// calling this method. A `DELETE` that is not resolved
([DeleteStatement#isResolved()]) did not come through such
+ /// an endpoint and is refused with a [QueryErrorCode#ACCESS_DENIED] error,
[#UNAUTHORIZED_DELETE_MESSAGE]. This
+ /// guard is a wiring check, not an authorization check: a resolved
statement is executed as is, so a new entry point
+ /// must authorize the caller as the query endpoints do.
+ ///
+ /// Execution failures are returned in the response, never thrown: a
[QueryException] thrown by the executing hook
+ /// is returned with its error code and message and logged without its stack
trace, any other exception is logged
+ /// with its stack trace and returned as a [QueryErrorCode#QUERY_EXECUTION]
error.
+ ///
+ /// @param statement parsed statement
+ /// @param headers headers of the original request, e.g. for minion task
submission
+ /// @return the response of the statement
+ public BrokerResponse executeStatement(DataManipulationStatement statement,
@Nullable Map<String, String> headers) {
BrokerResponseNative result = new BrokerResponseNative();
switch (statement.getExecutionType()) {
+ case EXECUTOR:
Review Comment:
Done in a8931cfa73: the legacy response is allocated inside the MINION,
HTTP, default and unsupported-executor branches only, so the DELETE path
allocates nothing it discards.
_🤖 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,563 @@
+/**
+ * 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.SqlDynamicParam;
+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;
+ }
+ };
+
+ /// Replaces every literal (string, numeric, date-time, binary, boolean,
interval) with a dynamic parameter, which
+ /// unparses as `?`, so that an error message renders the shape of a WHERE
clause without the values it compares
+ /// against: those may be data the caller of a request controls, and the
message is returned to the caller and
+ /// logged by the controller. A chain of string literals (`'a' 'b'`)
unparses its literals directly, so the chain is
+ /// replaced as a whole. The parameters all get the index 0, which the
rendering does not show.
+ private static final SqlShuttle LITERAL_MASKER = new SqlShuttle() {
+ @Override
+ public SqlNode visit(SqlLiteral literal) {
+ return new SqlDynamicParam(0, literal.getParserPosition());
+ }
+
+ @Override
+ public SqlNode visit(SqlCall call) {
+ if (call.getKind() == SqlKind.LITERAL_CHAIN) {
+ return new SqlDynamicParam(0, call.getParserPosition());
+ }
+ return super.visit(call);
+ }
+ };
+
+ /// 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`.
+ ///
+ /// Nothing derives this set from the function registries, so a function
that reads another table must be added
+ /// here when it is defined: `lookUp` is `TransformFunctionType#LOOKUP`
(with `LookupTransformFunction` in
+ /// pinot-core), `inSubquery` is rewritten by
`BaseSingleStageBrokerRequestHandler#IN_SUBQUERY` in pinot-broker, and
+ /// `inPartitionedSubquery` by
`ServerQueryExecutorV1Impl#IN_PARTITIONED_SUBQUERY` in pinot-core.
+ 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.
+ /// Like [#CROSS_TABLE_FUNCTIONS], it is maintained by hand: `groovy` is
`TransformFunctionType#GROOVY` (with
+ /// `GroovyTransformFunction` in pinot-core), and a new script function must
be added here.
+ private static final Set<String> SCRIPT_FUNCTIONS = Set.of("GROOVY");
+
+ /// Canonical names of the `LIKE` family, whose third operand is the escape
character. The operators `LIKE`,
+ /// `NOT LIKE`, `ILIKE` and `NOT ILIKE` compile to `LIKE` (Calcite gives
them the kind `LIKE`), `SIMILAR TO` to
+ /// `SIMILAR`, and a function-call spelling such as `like(col, pattern,
escape)`, which parses as a plain function,
+ /// to its own name. [#PREDICATE_NODE_CHECKER] rejects the operator form
`LIKE ... ESCAPE`, and [#checkFunctions]
+ /// the function-call form, which the node checker cannot tell from any
other function. The grammar bounds the
+ /// operator form to two or three operands, but the function-call form takes
any number: a call with a third or
+ /// later operand is rejected, as the single-stage filter reads the first
two operands only and silently drops the
+ /// others.
+ private static final Set<String> LIKE_FUNCTIONS = Set.of("LIKE", "NOTLIKE",
"ILIKE", "SIMILAR", "SIMILARTO");
+
+ private static final String LIKE_ESCAPE_MESSAGE = "Unsupported WHERE clause
in DELETE, LIKE ... ESCAPE is not "
+ + "supported in a DELETE predicate, the escape character is ignored by
the single-stage engine";
+
+ /// 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.
+ ///
+ /// The messages do not echo the literal, see [#LITERAL_MASKER]: the caller
sees it in their own statement.
+ 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");
+ 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");
+ } 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");
+ 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 loses
precision as a double: "
+ + "use CAST('<digits>' AS DECIMAL)");
+ }
+ }
+ }
+ 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.
The function-call form, like(col,
+ // pattern, escape), parses as a plain function: checkFunctions rejects
it on the compiled expression
+ if (call.getOperator() instanceof SqlLikeOperator) {
+ checkArgument(call.operandCount() != 3, LIKE_ESCAPE_MESSAGE);
+ }
+ return super.visit(call);
+ }
+ };
+
+ private final String _tableName;
+ private final String _predicate;
+ /// The expression [#_predicate] compiles to. [Expression] is mutable: never
handed out, see
+ /// [#getPredicateExpression()].
+ private final Expression _predicateExpression;
+ @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, CompiledPredicate predicate,
@Nullable String database,
+ Map<String, String> options) {
+ this(tableName, predicate._sql, predicate._expression, database, options,
false, false, false);
+ }
+
+ private DeleteStatement(String tableName, String predicate, Expression
predicateExpression,
+ @Nullable String database, Map<String, String> options, boolean
resolved, boolean tableExists,
+ boolean logicalTable) {
+ _tableName = tableName;
+ _predicate = predicate;
+ _predicateExpression = predicateExpression;
+ _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` (as an operator or as
+ /// the function call `like(col, pattern,
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");
+ CompiledPredicate 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;
+ }
+
+ /// A WHERE clause serialized into Pinot SQL, with the expression that SQL
compiles to, see [#toPinotSql].
+ private static final class CompiledPredicate {
+ private final String _sql;
+ private final Expression _expression;
+
+ private CompiledPredicate(String sql, Expression expression) {
+ _sql = sql;
+ _expression = expression;
+ }
+ }
+
+ /// Serializes a WHERE clause back into SQL, and verifies that Pinot parses
it into the same expression. Returns the
+ /// SQL with the expression it compiles to, which is the one the check
validated.
+ private static CompiledPredicate 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);
+ } 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
+ if (!serializedExpression.equals(expression)) {
+ throw new IllegalArgumentException(
+ "Unsupported WHERE clause in DELETE, it cannot be serialized back
into the same expression: "
+ + describe(condition));
+ }
+ checkFunctions(serializedExpression);
+ return new CompiledPredicate(predicate, serializedExpression);
+ }
+
+ /// Renders a node for an error message with its literals replaced by `?`,
see [#LITERAL_MASKER], and 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.accept(LITERAL_MASKER).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,
+ /// the functions that run a script, see [#SCRIPT_FUNCTIONS], the window
functions, which
+ /// [#PREDICATE_NODE_CHECKER] already rejects on the parsed node, and a
`LIKE` with an escape character (a third or
+ /// later operand) written as a function call, see [#LIKE_FUNCTIONS].
Function names are folded with
+ /// `toUpperCase(Locale.ROOT)`, see
+ /// [#CROSS_TABLE_FUNCTIONS], and a function name with non-ASCII characters
is rejected outright: no supported
+ /// function has one, and a look-alike spelling (e.g. with a dotless `ı`)
may be resolved case-insensitively to the
+ /// real function by an executor.
+ private static void checkFunctions(Expression expression) {
+ if (expression.getType() != ExpressionType.FUNCTION) {
+ return;
+ }
+ Function function = expression.getFunctionCall();
+ String operator = function.getOperator();
+ String functionName = operator.replace("_", "").toUpperCase(Locale.ROOT);
+ checkArgument(!CROSS_TABLE_FUNCTIONS.contains(functionName),
+ "Unsupported WHERE clause in DELETE, %s reads another table",
operator);
+ checkArgument(!SCRIPT_FUNCTIONS.contains(functionName),
+ "Unsupported WHERE clause in DELETE, %s is not supported in a DELETE
predicate", operator);
+ checkArgument(!WINDOW_FUNCTIONS.contains(functionName),
+ "Unsupported WHERE clause in DELETE, window functions are not
supported in a DELETE predicate");
+ checkArgument(!LIKE_FUNCTIONS.contains(functionName) ||
function.getOperandsSize() < 3, LIKE_ESCAPE_MESSAGE);
+ checkArgument(operator.chars().allMatch(c -> c < 128),
+ "Unsupported WHERE clause in DELETE, non-ASCII function name: %s",
operator);
+ 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 is not a
logical table with [#isLogicalTable()] and
+ /// that it 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. The
+ /// `database` option alone does not qualify an unqualified table name: the
broker's single-stage engine resolves
+ /// the table of a query from the `database` header only, so `SET database =
'db1'; DELETE FROM t` would delete
+ /// from another table than `SET database = 'db1'; SELECT ... FROM t` reads
on that engine. Such a statement is
+ /// rejected, and the caller sends the `database` header or qualifies the
table name as `database.table`.
+ ///
+ /// @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
`database` option is set without the `database`
+ /// header on an unqualified table name
+ public DeleteStatement resolveTableName(@Nullable String databaseHeader,
TableCache tableCache)
+ throws QueryException {
+ if (_database != null && databaseHeader == null &&
!_tableName.contains(".")) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException("The database option
does not apply to a DELETE with an "
+ + "unqualified table name, since the single-stage engine resolves a
query from the database header only: "
+ + "send the database header or qualify the table name as
database.table");
+ }
+ String database =
DatabaseUtils.extractDatabaseFromOptionAndHeader(_database, databaseHeader);
+ // getTableName already rejected the names translateTableName does not
accept
+ String tableName = DatabaseUtils.translateTableName(_tableName, database,
tableCache.isIgnoreCase());
+ String actualTableName = tableCache.getActualTableName(tableName);
+ boolean logicalTable = actualTableName == null &&
tableCache.getActualLogicalTableName(tableName) != null;
+ return new DeleteStatement(actualTableName != null ? actualTableName :
tableName, _predicate,
+ _predicateExpression, null, _options, true, actualTableName != null,
logicalTable);
Review Comment:
Done in a8931cfa73: the parsing constructor is the one place the options are
copied and frozen; the resolved statement reuses that immutable map instead of
copying and wrapping it again.
_🤖 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]
