morningman opened a new pull request, #67966:
URL: https://github.com/apache/doris/pull/67966
### What problem does this PR solve?
Issue Number: #67578
Related: #67577
Related PR: #67900 (the Flight session lock and per-statement hooks this
builds on)
Problem Summary:
First server-side item of #67578 (A1). The Arrow Flight SQL session actions
were left at the Arrow Java defaults: `SetSessionOptions` and
`GetSessionOptions` answered `UNIMPLEMENTED`, and `CloseSession` was
implemented but answered twice when it failed. So a client had no way to set
the session's catalog, database or a session variable other than sending `SET`
/ `USE` / `SWITCH` as SQL statements, and the ADBC connection options
`adbc.connection.catalog` / `adbc.connection.db_schema`
(`conn.adbc_current_catalog` / `conn.adbc_current_db_schema` in Python) failed
on Doris, as did the Flight SQL JDBC driver's `catalog` connection property.
This PR implements the three actions on the Doris session:
```
ADBC driver (Go; Python / C++ / R bindings) Flight SQL JDBC
driver 19
adbc.connection.catalog --> session option "catalog" <-- URL
property ?catalog=... (set at connect)
adbc.connection.db_schema --> session option "schema"
adbc.flight.sql.session.option.<name> / optionbool. / optionerase. -->
session option <name>
Connection.Close() --> CloseSession <--
Connection.close()
|
v
DorisFlightSqlProducer.setSessionOptions / getSessionOptions (one
command of the session, under its lock)
|
v
FlightSessionOptions: "catalog" -> SWITCH `x` "schema" -> USE `x`
<name> -> SET SESSION name = <literal>
run through FlightSqlConnectProcessor.handleQuery:
checked, audited, takes effect
exactly as if the client had sent the statement
get(): "catalog", "schema" + every row of SHOW
VARIABLES, values as strings
```
**Keys.** `catalog` and `schema` are the two names the ADBC Flight SQL
driver puts on the wire for the connection options (its
`driverbase.CurrentNamespacer` translates `adbc.connection.catalog` into
`SetSessionOptions({catalog: x})`; the constants in `adbc.h` are
`adbc.connection.catalog` / `adbc.connection.db_schema`, not the `current_*`
spelling in #67578's A1 text) and the one the JDBC driver sends for its
`catalog` property. Every other name is a session variable, as the drivers pass
`adbc.flight.sql.session.option.<name>` through unchanged. The
`adbc.connection.*` names themselves are not accepted: no client sends them.
**SetSessionOptions.** Each option is set on its own and answered on its
own, as the spec asks; the action fails as a whole only when the session cannot
be reached (or is busy: the same `UNAVAILABLE` as any other command of the
session). Within one request `catalog` is applied first and `schema` second,
since `USE` names a database of the current catalog. The value is rendered as
the literal of its own type - a string quoted for the session's `sql_mode` (so
`NO_BACKSLASH_ESCAPES` round-trips), `true` / `false`, an integer, a double;
the empty value that a driver sends to erase an option becomes `SET name =
DEFAULT`; a string list has no literal. Per-name results:
| result | when |
|---|---|
| `INVALID_NAME` | the name is not an identifier or not a session variable
(`VariableMgr.getVarContext` finds nothing; a removed variable or a
MySQL-compatibility-whitelist name that `SET` silently ignores is no variable
of the session either) |
| `INVALID_VALUE` | a string list, a NaN / infinite double, an empty
`catalog` / `schema`; or the statement refused the value: `SET` rejected it
(type, range, the variable's own checker, `time_zone` validation), the catalog
or database does not exist |
| `ERROR` | the variable is read-only or global-only (`SET GLOBAL`
territory), or `USE` / `SWITCH` failed for another reason, e.g. no privilege |
Why the split is done this way: `StmtExecutor.execute` rewrites the error
code of a failed command to `ERR_UNKNOWN_ERROR` (the two ERR cases in the
Flight golden show it), so a `SET` failure cannot be classified afterwards. The
name and the scope are therefore checked before the statement runs, which
leaves the value as the only thing `SET` can still refuse. `USE` and `SWITCH`
write `ERR_BAD_DB_ERROR` / `ERR_UNKNOWN_CATALOG` onto the session state
themselves without throwing, so those two survive and mean "the value"; a
validation failure (privilege) is rewritten and means `ERROR`. Doris checks the
privilege before the existence of a catalog or database, and the option keeps
that order rather than pre-checking existence. The result carries no message
per name (the protocol has none), so the reason a value was refused is logged
at WARN with the statement.
**GetSessionOptions.** `catalog` = the current catalog, `schema` = the
current database (`""` when none), and every session variable `SHOW VARIABLES`
lists, in its text - one representation, the one `SET` accepts back, whatever
the Java type behind the variable (`sql_mode` is a `long` that is set and shown
as names). This is the mapping DSIP-YYY / #67578 A1 give (`GetSessionOptions` ↔
`SHOW VARIABLES`). The consequence for the ADBC driver: read a variable with
the string getter (`get_option`), not `get_option_int`; the driver reports a
type mismatch otherwise.
**CloseSession** already invalidated the bearer token at once
(`FlightTokenManagerImpl.invalidateToken` unregisters the `ConnectContext`
synchronously, and any later call with the token is `UNAUTHENTICATED` at the
authenticator) - verified rather than changed; the fix here is that a failure
now answers `onError` alone instead of `onError` followed by `CLOSED`. The
stale comment claiming that ADBC never calls `CloseSession` is gone: the Go
driver's `Connection.Close()` always calls it, and so does the JDBC driver's
`Connection.close()`.
Type mapping is untouched, per the deferral note under Stage 2 of #67577:
session options carry no Arrow schema.
### Release note
Arrow Flight SQL: the standard session actions are served.
`SetSessionOptions` sets the current catalog (`catalog`), the current database
(`schema`) and session variables (any other name), each answered per name with
`INVALID_NAME` / `INVALID_VALUE` / `ERROR`; `GetSessionOptions` returns them
with every session variable as `SHOW VARIABLES` shows it. With the ADBC Flight
SQL driver this makes `adbc.connection.catalog` / `adbc.connection.db_schema`
and `adbc.flight.sql.session.option.<variable>` work; with the Flight SQL JDBC
driver the `catalog` connection property is applied at connect time (and a
connection whose catalog cannot be set now fails instead of being silently
accepted). `CloseSession` unchanged: the bearer token is invalid at once.
### Check List (For Author)
- Test
- [x] Regression test
- [x] Unit Test
- [x] Manual test (add detailed scripts or steps below)
**Unit tests.** `FlightSessionOptionsTest` (in-process FE, real `SET` /
`USE` / `SWITCH`): the three results, per key; typed and string values; the
empty value restoring the default; quoting under `NO_BACKSLASH_ESCAPES`;
case-insensitive names; the privilege-before-existence order for a user without
privileges; a whole request; and the read-back equal to `VariableMgr.dump`.
`DorisFlightSqlProducerTest`: the listener plumbing of the three actions, an
action waiting for the session's running command and giving up with
`UNAVAILABLE`, `CloseSession` answering once. Protocol goldens
`MysqlPacketGoldenTest` / `FlightResultGoldenTest` unchanged.
**Regression.** `arrow_flight_sql_p0/test_session_options`: drives the
actions with the `FlightSqlClient` shaded inside the Flight SQL JDBC driver on
the classpath (same auth2 handshake as the drivers), checks that `schema` makes
`SHOW TABLES` work in the session, that variables set as options are what `SHOW
VARIABLES` shows, the per-name results, string round-trip, the token being
refused after `CloseSession`; and, black-box, that the JDBC driver connects
with `?catalog=internal` and refuses `?catalog=no_such_catalog` with `Cannot
set session option for catalog`. Whole `arrow_flight_sql_p0` green locally: 10
suites, 0 failed (the docker suite skipped as always), against an FE and a BE
both built from this branch.
**Manual: official ADBC driver** (`adbc_driver_flightsql` 1.12.0, Python):
```
adbc_driver_flightsql 1.12.0 adbc_driver_manager 1.12.0
adbc_current_catalog = 'internal'
adbc_current_db_schema = ''
after set: adbc_current_db_schema = 'adbc_opts_db'
select database() -> [('adbc_opts_db',)]
after set: adbc_current_catalog = 'internal'
query_timeout (string set) = '77'
query_timeout (int set) = '88'
select @@query_timeout -> [(88,)]
enable_profile = 'true'
query_timeout (erased) = '900'
unknown name: ProgrammingError: INVALID_ARGUMENT: [Flight SQL] Could not
set option(s) 'no_such_variable' (invalid name)
unknown schema: ProgrammingError: INVALID_ARGUMENT: [Flight SQL] Could not
set option(s) 'schema' (invalid value)
unknown catalog: ProgrammingError: INVALID_ARGUMENT: [Flight SQL] Could
not set option(s) 'catalog' (invalid value)
bad value: ProgrammingError: INVALID_ARGUMENT: [Flight SQL] Could not set
option(s) 'query_timeout' (invalid value)
read-only variable: ProgrammingError: INVALID_ARGUMENT: [Flight SQL] Could
not set option(s) 'net_buffer_length' (error setting option)
get_option_int on a variable: ProgrammingError: NOT_FOUND: [Flight SQL]
session option query_timeout="900" is not an integer value
all session options: 19040 bytes of JSON, starts with
{"DML_PLAN_RETRY_TIMES":"3","adaptive_pipeline_task_serial_r
closed
```
The `closed` line is `conn.close()`; fe.log shows the `Invalidate bearer
token` of the `CloseSession` it sent, and the session is gone from `SHOW
PROCESSLIST`. The `get_option_int` line is the documented consequence of
returning `SHOW VARIABLES` text: the driver's typed getter refuses it, the
string getter is the one to use.
- Behavior changed:
- [x] Yes. `SetSessionOptions` / `GetSessionOptions` answer instead of
`UNIMPLEMENTED`; a JDBC connection with a `catalog` property that cannot be set
now fails to connect (the driver treats the server's error as fatal, and before
this PR the server's `UNIMPLEMENTED` was swallowed).
- Does this need documentation?
- [x] Yes. Follow-up on the Arrow Flight SQL page: the session options
and the ADBC / JDBC options that map onto them.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01Ja986K4PEm2LD8u44L9jd2
--
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]
