xiangfu0 opened a new pull request, #19664:
URL: https://github.com/apache/pinot/pull/19664
Labels: `feature`, `backward-incompat`, `release-notes`, `extension-point`
## Summary
`CalciteSqlParser.extractSqlNodeAndOptions` only classified
`SqlInsertFromFile` as DML, so a Calcite `SqlDelete`
(`DELETE FROM t WHERE ...`, which the grammar already parses) fell through
to the DQL branch. It was then compiled as
a query and rejected with a confusing error (`ClassCastException` to
`SqlSelect` in the single-stage compiler,
"Unsupported SQL query" in the multi-stage planner, the same cast failure on
the controller `/sql` DQL path).
This change makes Pinot parse `DELETE` as DML and lets a deployment plug in
how rows are deleted. Pinot itself does
not delete rows: the default executor answers that `DELETE` is not supported.
- `CalciteSqlParser` classifies `SqlDelete` as `PinotSqlType.DML`, so the
broker (`/query/sql`, `/query`) and the
controller (`/sql`) hand it to their `SqlQueryExecutor`, like `INSERT INTO
... FROM FILE`.
- New `DeleteStatement` (`org.apache.pinot.sql.parsers.dml`), returned by
`DataManipulationStatementParser` for
`DELETE`:
- `getTableName()`: `table` or `database.table` as written.
- `getPredicate()`: the WHERE clause serialized back into Pinot SQL. It
must parse
(`CalciteSqlParser.compileToExpression`) into the same expression as the
statement, or the statement is rejected.
Literal quotes are escaped. Identifiers are quoted where they were
quoted, and where needed to keep their name:
Calcite unparses identifiers named like SQL functions without arguments
(`user`, `pi`, `current_date`, ...) as
upper-cased keywords, so they are quoted. The predicate is parsed but
not validated as a filter of the table
(columns, aggregations): the executor validates it.
- `getDatabase()`: from the `database` option, e.g. `SET database =
'...'`. Executors combine it with the
`database` header the way queries do.
- `getOptions()`: the statement's own options, from `SET` statements and
the request `queryOptions` (`SET` takes
precedence), i.e. without the database and the query and request options.
`DELETE` without a WHERE clause, with a table alias, or with a WHERE
clause Pinot cannot parse as an expression
(e.g. a subquery) is rejected. So is another spelling of the `database`
option (e.g. `SET DATABASE = ...`):
queries only read `database`, so a `DELETE` must not read another one.
- `SqlQueryExecutor.executeDMLStatement` hands a `DeleteStatement` to a new
`protected BrokerResponse executeDelete(DeleteStatement statement,
@Nullable Map<String, String> headers)`. The
default answers with a `QUERY_VALIDATION` error ("DELETE is not supported
by this Pinot cluster"). Like other DML,
the statement reaches the executor without table-level authorization or
query logging, so implementations
authorize the caller with the request headers, as documented on the method.
- `BaseBrokerStarter` and `BaseControllerStarter` create their executor with
a new
`protected SqlQueryExecutor createSqlQueryExecutor()`. Deployments that
can delete rows, e.g. by purging the matching
rows from the segments with a minion task, override it to return a
`SqlQueryExecutor` subclass implementing
`executeDelete`.
- `SqlQueryExecutor.executeDMLStatement` answers a DML statement it cannot
parse with a `SQL_PARSING` error response
instead of throwing, which surfaced as an HTTP 500 on the broker.
- New `QueryOptionsUtils.isQueryOptionKey(String)`: whether a key, ignoring
case, is a query or request option rather
than an option of a statement: a `QueryOptionKey`, `trace`, `database`,
the legacy `groupByMode` and
`responseFormat` that the Java client still sends with every request, or a
key registered with
`registerSqlQueryOptionKey`. `DeleteStatement` uses it to tell its options
from query options. Registered keys are
per process, so brokers and controllers should register the same ones.
- The options of a DML statement configure it (e.g. a dry run, or the
`taskName` of `INSERT INTO ... FROM FILE`), so
dropping them would silently change what it does. A DML statement with SQL
options now fails with
`QUERY_VALIDATION` when the request sets `sqlOptionsMode=IGNORE`, and a
DML statement using the legacy
`OPTION(...)` syntax fails to parse on a broker configured to ignore that
syntax
(`pinot.broker.query.option.legacySyntaxMode=IGNORE`). Queries are
unaffected. The docs of both settings say so.
- The controller `GET /sql` rejects DML with a `QUERY_VALIDATION` error, as
the broker `GET /query/sql` already rejects
it (`onlyDql`): a GET must not modify data, e.g. when a browser holding
credentials follows a crafted link.
`POST /sql` still executes DML. This also applies to `INSERT INTO ... FROM
FILE`, which a GET could previously run.
`EXPLAIN PLAN FOR DELETE ...` is unchanged (still an explain statement).
**Backward incompatible:**
- Clients that submit `INSERT INTO ... FROM FILE` with the controller `GET
/sql` must switch to `POST /sql`.
- Clients that send DML statements with `sqlOptionsMode=IGNORE`, or with
`OPTION(...)` on a broker that ignores that
syntax, must drop that mode or move the options to the request.
## Testing
- `DeleteStatementTest`:
- parsing of the table, with database and quoted names;
- the options split: query options, request options such as `trace` and
`groupByMode`, registered keys and the
database are excluded; request `queryOptions` are included;
- rejections: no WHERE, alias, `a.b.c`, subquery, `SET DATABASE`;
- dispatch through `DataManipulationStatementParser`;
- 20 WHERE clause round trips that compile into the same filter expression
as the original: quotes, unicode,
reserved words, `IN`, `BETWEEN`, `LIKE`, `IS NULL`, functions, `CAST`,
`JSON_MATCH`, `TEXT_MATCH`, timestamps,
`CASE`, and columns named `user`, `pi`, `current_date`,
`current_timestamp` in any case;
- the exact serialization of such columns.
- `SqlQueryExecutorTest`:
- `DELETE` on the default executor returns a `QUERY_VALIDATION` "not
supported" error without contacting the
controller;
- an invalid `DELETE` returns `SQL_PARSING`;
- an executor overriding `executeDelete` receives the parsed statement and
the request headers.
- `QueryOptionsUtilsTest#testIsQueryOptionKey`.
- `SqlOptionsModeTest`:
- DML with SQL options fails under `sqlOptionsMode=IGNORE`, for `SET` and
`OPTION(...)`, `DELETE` and
`INSERT INTO ... FROM FILE`;
- DML without SQL options keeps the request options;
- DML with `OPTION(...)` fails to parse when the legacy syntax is ignored,
while `SET` keeps working.
- `CalciteSqlCompilerTest#testDeleteIsClassifiedAsDml`: classification, SET
options, target table and condition, and
a DELETE cannot be combined with another executable statement.
- `PinotQueryResourceTest#testDmlOnGetQueryEndpointReturnsValidationError`
and
`#testDmlOnPostQueryEndpointIsExecuted`: GET /sql rejects DELETE and
INSERT INTO FILE; POST /sql dispatches DELETE
to the DML executor.
- These pass, along with `RequestUtilsTest`, `InsertIntoFileTest`,
`PinotDdlParserTest`,
`BaseSingleStageBrokerRequestHandlerTest` and `PinotClientRequestTest`.
Checkstyle, spotless and license checks
pass.
- A downstream executor that overrides `executeDelete` and
`createSqlQueryExecutor` runs end to end against this
change: `DELETE` through the broker and the controller, dry run, repeated
deletes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_019Gm6pH91CjfJQ4kWdQBLac
--
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]
