Repository navigation
[fix](flight-sql) Return analyzed result schemas from Prepare and GetSchema - #69867
Conversation
…command ### What problem does this PR solve? Issue Number: None Related PR: apache#68631 Problem Summary: branch-4.1 apache#68631 (also on branch-4.2) makes Flight SQL Prepare and GetSchema advertise the real result schema of a statement. For a command that answers from the frontend, that schema is its result header. Several commands only pick their header while they run, by what they find, and master has no way to ask them for it without running them: - HELP returns a topic, keyword or category header depending on what the help catalog matches. - SHOW SNAPSHOT returns the detail header only when both the SNAPSHOT and TIMESTAMP filters are given, after validating them. - SHOW CREATE TABLE returns a table, view, stream or materialized view header depending on the table. - SHOW PROC builds its header from the node it opens. - SHOW CONFIG and WARM UP SELECT return rows, but their `getResultSetMetaData()` is the empty default of `Command`. This commit adds, as apache#68631 does, a way to resolve each header without running the command: `HelpCommand.getMetaData(ConnectContext)` (the help catalog's matches, without reading topic text), `ShowSnapshotCommand.getMetaData(ConnectContext)` (validates the filters, never touches the repository, and is now what `handleShowSnapshot` uses), `ShowCreateTableCommand.getMetaData(ConnectContext)`, `ShowProcCommand.getMetaData(ConnectContext)` (the privilege check, then the static headers of `/` and `/current_queries`, and null for any other node, since opening one can reach remote nodes), the `getResultSetMetaData()` overrides of `ShowConfigCommand` and `WarmupSelectCommand`, and catalog / database getters on `UseCommand`. Where master differs from branch-4.x: - master's SHOW CREATE TABLE has more header branches than 4.x (stream tables, the sys-table redirect, plugin-driven tables and views). The header choice of `doRun` is factored out into one method that the new `getMetaData(ConnectContext)` shares, instead of a second copy. - master has no SHOW CREATE PROCEDURE (PL/SQL was removed in apache#58700), and its SHOW PROCEDURE STATUS already reports its header as a ShowCommand. ### Release note None ### Check List (For Author) - Test: Unit Test - `SchemaOnlyCommandMetadataTest` (from apache#68631): the HELP headers match what `doRun` returns for a topic, a keyword with one or several topics, one or several categories and no match, without reading the topic; SHOW SNAPSHOT resolves its header without the backup handler and rejects an invalid filter - `ShowCreateTableCommandTest`, `ShowSnapshotCommandTest`, `ShowConfigCommandTest` still pass - Behavior changed: No - Does this need documentation: No Co-authored-by: Gabriel <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: apache#68631, apache#67577 Problem Summary: `DorisArrowTypeMapping` is the one place FE turns a Doris type into an Arrow type. It describes TIMEV2 and VARBINARY as Arrow Null, while BE sends float64 and binary for them (`convert_to_arrow_type` in `be/src/format/arrow/arrow_row_batch.cpp`). Its class comment and `DorisArrowTypeMappingTest` record these cells as known to disagree with BE and kept until the BE Arrow type layer is reworked (apache#67577). The next commit makes Flight SQL Prepare advertise the analyzed result schema from this mapping and checks every prepared execution against the schema BE returns. With the two Null cells, every prepared execution of a TIME or VARBINARY result would be rejected as a schema change. branch-4.1 apache#68631 (also on branch-4.2) corrects the same two cells in its shared type mapping. TIMEV2 now maps to Arrow Double and VARBINARY to Arrow Binary, as BE sends them. GetTables reports the same types in its `table_schema` for such columns. TIMESTAMPTZ (literal "UTC" where BE stamps the session time zone) and AGG_STATE (Null) stay as they are; Prepare handles those two on its own path. ### Release note Arrow Flight SQL GetTables describes TIME and VARBINARY columns as Arrow Double and Binary, the types the results carry, instead of Null. ### Check List (For Author) - Test: Unit Test - `DorisArrowTypeMappingTest`: the TIMEV2 and VARBINARY rows are updated; the other rows and `test_get_tables_schema` (no TIME or VARBINARY column) are unchanged - Behavior changed: Yes. GetTables `table_schema` reports Double for TIME columns and Binary for VARBINARY columns - Does this need documentation: No Co-authored-by: Gabriel <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
…Schema ### What problem does this PR solve? Issue Number: None Related PR: apache#68631, apache#68768 Problem Summary: On master, Flight SQL `CreatePreparedStatement` registers the SQL and answers with the same one-column `ResultMeta: Utf8` dataset schema for every statement, without analyzing it, and `GetSchema` for a statement or a prepared statement is UNIMPLEMENTED. A client that reads the prepared schema gets wrong columns: Python ADBC `adbc_execute_schema()` uses it directly. An invalid statement also prepares successfully and only fails when it runs. branch-4.1 apache#68631 (also on branch-4.2) fixes this. This commit ports apache#68631 onto master's Flight session layer: - `FlightSqlQuerySchema` resolves the result schema with the FE-only `PrepareCommandPlanner` and `CheckPrivileges`, without building an executor or sending fragments. It works on a clone of the session variables, applies the dialect conversion and the original-SQL parse fallback as execution does, resolves USE / SWITCH prefixes in a namespace it puts back afterwards, and builds the Arrow fields as BE does, with the `doris_type` markers execution adds at every depth and native Variant fields. A command's schema is its result header, or the OK `StatusResult` for the no-row command categories and the concrete DML commands. - Prepare analyzes before it registers a handle, so a statement that cannot run leaves none behind. Prepare fails with INVALID_ARGUMENT for invalid SQL or a missing privilege. Otherwise the handle keeps the SQL, the namespace it was prepared in and the advertised schema. - A prepared execution analyzes the SQL again and rejects a schema different from the advertised one, for example after a `sql_mode` change. After execution it compares the schema of the actual result too. On a mismatch it drops the handle, cancels the query, drops what the request left (deferred coordinator, cached result, endpoints) and fails with NOT_FOUND. A handle used in another catalog or database expires; a USE or SWITCH in the statement itself rebinds only that handle. - GetSchema for a statement and for a prepared statement return the analyzed schema. Where master differs from branch-4.x: - Master serializes the commands of a session with `FlightProtocolAdapter.callCommand`. The analysis runs inside it, instead of the `synchronized (connectContext)` blocks of 4.x, so execution cannot touch the analysis's temporary session state. The prepared statements are kept in `FlightProtocolAdapter` as `PreparedQuery`, where master keeps them, and `ConnectContext` only forwards. The namespace is captured and restored with `NameSpaceContext`, which does not contact the catalog. - A statement whose result schema only running it can tell keeps today's `ResultMeta` placeholder on Prepare, and its executions are not checked. That covers DESCRIBE, SHOW DATA / PARTITIONS / QUERY STATS / PYTHON PACKAGES, PROC nodes other than `/` and `/current_queries`, COPY INTO, WARM UP CLUSTER, a request whose statements before the last are not USE or SWITCH, `?` placeholders, and a result type FE cannot describe as BE sends it. 4.x answers UNIMPLEMENTED there. Python ADBC then executes without preparing, but the Arrow Flight SQL JDBC driver prepares every statement and does not fall back, so DESC and `SET @v = 1; SELECT @v`, which master's `test_arrow_flight_session_lifecycle` runs over JDBC, would fail. GetSchema for such a statement, or for such a handle, answers UNIMPLEMENTED. - EXPLAIN PLAN PROCESS has a Flight result on master (Rule / Before / After), so it gets that header; 4.x rejects it. PLAN REPLAYER (removed by apache#67811) and the Lance index commands are not on master. - SWITCH resolves the default database of a catalog without a database layer through the connector provider (`Env.defaultDatabaseOnUse`, now shared with `Env.changeCatalog`), where 4.x named the ES catalog. - TIMESTAMPTZ results carry the canonical session time zone that FE sends BE (`TimeUtils.getCanonicalTimeZone`), as BE stamps it. - Closing the analysis's `StatementContext` leaves the connector statement scope of a Flight statement to the query-finish cleanup, which never runs for an analysis, so the analysis closes that scope itself. - From branch-4.2 apache#68768, which is otherwise left to the next batch (it adds parameter binding): a prepared statement handle must carry the calling session's peer identity and a non-empty id. Get-flight-info, get-schema and close of a prepared statement all check this, and the session comes from the caller rather than from the handle. Each Prepare, and each execution of a prepared statement whose schema was advertised, runs one more FE analysis of the statement. Physical planning and execution are unchanged. ### Release note Arrow Flight SQL Prepare and GetSchema return the real result schema of a query instead of a placeholder, and a prepared statement whose result schema changes must be prepared again. ### Check List (For Author) - Test: Unit Test - `DorisFlightSqlSchemaTest` (from apache#68631, on `ConnectContext.forFlight`): analyzed schemas, privileges, namespaces, schema drift, command headers, the placeholder for statements that cannot be told, and the handle checks - `FlightSqlExecutionSchemaTest` (from apache#68631): the analyzed schema matches the labels and types of a real `handleQuery` - `DorisFlightSqlProducerTest#createPreparedStatementDoesNotLeakChannelAllocator` now prepares statements that keep the placeholder and still checks the channel allocator returns to zero - Behavior changed: Yes. Prepare fails for invalid SQL or a missing privilege instead of the execution; a prepared statement expires when its schema changes or it is used in another namespace - Does this need documentation: No Co-authored-by: Gabriel <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: apache#68631 Problem Summary: branch-4.1 apache#68631 (also on branch-4.2) adds `test_schema_only`, which drives Prepare, GetSchema and prepared execution through a plain Flight SQL client against a cluster. This commit brings it to master: - Unaliased literals, functions and CASTs keep the labels execution gives them. - Analyzed schemas for typed, nested, information_schema and FE-local results, and the headers of the commands that resolve them without running. - `sql_mode` drift expires a prepared handle. - SET and SET_VAR hints are not applied by schema discovery. - Invalid SQL is rejected with INVALID_ARGUMENT. - A handle that changes its own namespace stays usable, while a handle used in another namespace expires. Adapted to master: the SHOW CREATE PROCEDURE and PLAN REPLAYER items are dropped (neither exists on master), EXPLAIN PLAN PROCESS is checked for its Rule / Before / After header, and statements whose schema only running them can tell (SHOW PYTHON PACKAGES, SHOW QUERY STATS, DESC, `SET ...; SELECT ...`) are checked to keep the placeholder on Prepare, answer UNIMPLEMENTED on GetSchema and still run. The `group_concat_state` case also executes, so the advertised UTF8 type is checked against what BE sends. ### Release note None ### Check List (For Author) - Test: Regression test - `arrow_flight_sql_p0/test_schema_only` - Behavior changed: No - Does this need documentation: No Co-authored-by: Gabriel <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
…verflow ### What problem does this PR solve? Issue Number: None Related PR: apache#68631 Problem Summary: `FeServiceClient`, which reads the metadata of a remote Doris catalog, takes the catalog's timeout in seconds and gives the pooled Thrift sockets milliseconds. master already converts the unit (apache#60557), but with `(int) TimeUnit.SECONDS.toMillis(timeoutS)`, so a timeout over about 24 days wraps around to a negative or small socket timeout. branch-4.1 apache#68631 (also on branch-4.2) converts with an overflow check. This commit does the same with `Math.toIntExact`, so such a value fails when the client is built. ### Release note None ### Check List (For Author) - Test: Unit Test - `FeServiceClientTest` (from apache#68631): `Integer.MAX_VALUE` seconds is rejected, and a metadata call answered by a local Thrift server after 100 ms succeeds under a 10 second timeout - Behavior changed: No - Does this need documentation: No Co-authored-by: Gabriel <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed exact head b4f86e886005b8323596ea8b74ef1c0b9f4e9e2c. Three P2 correctness regressions are reported inline: a single-use CTE can fail Prepare under column-level grants; ordinary SHOW STORAGE POLICY advertises the wrong header; and an empty-input WARM UP SELECT returns OK instead of its advertised statistics header. These cases should be fixed and covered by prepared-execution tests.
Critical checkpoint conclusions:
- Goal: analyzed Prepare/GetSchema and prepared-schema validation are implemented, with the three exceptions above. The unit and Flight regression tests were inspected.
- Scope and reuse: discovery reuses PrepareCommandPlanner, shared Arrow mapping/metadata helpers, namespace snapshots and the session command lock. The independent timeout conversion rejects int-millisecond overflow.
- Concurrency: action workers and RPC commands serialize through the session command lock, protecting the prepared map, borrowed context and handle namespace. Existing bounded, ID-ordered table locking is preserved; no new lock-order defect was found.
- Lifecycle: analysis closes its StatementContexts/connector scopes and restores captured state in finally. Schema-only requests allocate schema objects rather than Arrow result vectors. Mismatch cancellation and result/deferred-resource cleanup precede ticket publication.
- Configuration: none added; relevant existing settings are re-read for analysis and execution.
- Compatibility: dynamic/unsupported statements retain placeholder preparation and unchecked execution. Cloud and non-cloud use shared paths. The inline findings identify existing behavior rejected by the new checks.
- Thrift/protobuf: no gensrc definitions or field IDs changed.
- Parallel paths: reviewed statement/prepared GetSchema, Prepare, checked/placeholder execution, FE command/BE query results, namespace prefixes and shared MySQL command helpers. TIMEV2/VARBINARY and recursive logical metadata agree with the BE converters.
- Conditional/error checks: schema mismatches fail before publication and unknown schemas fall back explicitly; two reported command paths incorrectly claim a stable header. The CTE check runs before required-column handling is equivalent to execution.
- Test coverage: invalid SQL, grants, session restoration, namespace/SQL-mode drift, caller handles, nested types, labels/nullability, cleanup, allocator use and timeout conversion have coverage. The three concrete inline scenarios are missing.
- Test results: no golden output files changed. Expected schemas were compared with production metadata/conversion code. Tests were inspected, not executed; the author's reported passing runs were not independently reproduced.
- Observability: analysis errors retain their cause, schema expiry provides prepare-again guidance, and cancellation failures are logged. No separate logging/metrics defect was established.
- Persistence/transactions: no EditLog, storage-format, visible-version, delete-bitmap or transaction/failover protocol changes.
- Writes/atomicity: discovery does not execute commands; existing DML transactions remain in the ordinary executor.
- FE/BE state: no new transmitted variables. TIMESTAMPTZ follows the existing canonical session-zone convention.
- Performance: repeated analysis intentionally validates drift; no additional concrete allocation or complexity defect was found.
- Other concerns and user focus: no additional user-provided focus or existing inline findings. All changed files, candidates and risk focuses have dispositions; no concrete static coverage gap remains.
The review converged after two rounds: every assigned reviewer returned NO_NEW_VALUABLE_FINDINGS in round 2. Builds, tests, benchmarks and runtime Arrow/JDBC validation were not run under the static-only review contract.
Review converged after 2 round(s).
| CascadesContext cascades = planner.getCascadesContext(); | ||
| Plan analyzed = cascades.getRewritePlan(); | ||
| // PrepareCommandPlanner stops before the rewrite phase that normally checks privileges. | ||
| new CheckPrivileges().rewriteRoot(analyzed, cascades.getCurrentJobContext()); |
There was a problem hiding this comment.
[P2] Preserve required-column pruning for single-use CTEs
With only SELECT(id) granted on t(id, secret), WITH c AS (SELECT id, secret FROM t) SELECT id FROM c succeeds through ordinary planning: CTEInline removes the single deterministic reference's anchor, then CheckPrivileges prunes the unused secret. At this analyzed-only point the reduced tree is still:
CTEAnchor
Producer: Project(id, secret, Scan(t))
Project(id, Consumer(c))
ColumnPruning preserves the producer's outputs here, and CheckPrivileges.visitLogicalCTEConsumer explicitly checks its complete output bitmap, so this call demands SELECT(secret). Prepare/GetSchema now reject an otherwise valid query, and Flight JDBC fails before execution. Apply execution-equivalent CTE inlining/required-column handling before this check.
There was a problem hiding this comment.
Fixed in 7c7393c8c18. Confirmed. Prepare checked privileges on the analyzed plan, where the CTE is still a LogicalCTEAnchor over its producer and ColumnPruning treats every producer output as needed, so a CTE used once was checked on every column it selects. Execution first inlines that CTE (CTEInline, inline_cte_referenced_threshold = 1) and checks only the columns the query reads. The same applies to SELECT * in the CTE and to column-level policies of external catalogs. FlightSqlQuerySchema.checkPrivileges now runs PullUpCteAnchor and CTEInline on the analyzed plan before CheckPrivileges, as the rewriter does before its own check. A CTE used more than once is still checked on its full output, and execution does the same (TestCheckPrivileges pins that). Inlining alone does not make the two checks equivalent, because the rewriter normalizes the plan further before it checks privileges. For instance, it drops an ORDER BY under a derived table, so SELECT id FROM (SELECT id, secret FROM t ORDER BY secret) d runs for a user with only SELECT_PRIV(id), while its analyzed plan still reads secret. A denial on the analyzed plan therefore no longer fails Prepare. The statement keeps the placeholder schema and runs unchecked, so execution decides as it did before this PR, and GetSchema answers UNIMPLEMENTED with the denial and no schema. DorisFlightSqlSchemaTest#columnGrantsAreCheckedAsExecutionChecksThem grants SELECT_PRIV(id) and checks each statement against the rewriter (PlanChecker). Four single-use CTE shapes return [id] from GetSchema and Prepare. SELECT name, the CTE used twice and the derived-table ORDER BY keep the placeholder. The test fails without the inlining, and schemaDiscoveryChecksTablePrivileges fails if a denial fails Prepare again.
| } | ||
| return stringMetadata("Explain String(Nereids Planner)"); | ||
| } | ||
| ResultSetMetaData metadata = command.getResultSetMetaData(); |
There was a problem hiding this comment.
[P2] Resolve the ordinary SHOW STORAGE POLICY header
For SHOW STORAGE POLICY, ShowStoragePolicyCommand.getMetaData() always returns the four-column USING header, while doRun() delegates to PolicyMgr.showStoragePolicy() and returns the seven-column StoragePolicy.STORAGE_META_DATA, even when there are no policy rows. This generic getter therefore advertises four columns at Prepare, and executePreparedStatement subsequently rejects every execution as a schema change. Flight JDBC prepares every statement, so the command stops working through that driver. Resolve the ordinary versus USING header here, or retain the placeholder until the correct header is available.
There was a problem hiding this comment.
Fixed in 7c7393c8c18. Confirmed, and the wrong header predates this PR. Since #44393, ShowStoragePolicyCommand.getMetaData() has returned the four-column USING header for every form. Plain SHOW STORAGE POLICY returns PolicyMgr.showStoragePolicy(), whose header is the seven-column StoragePolicy.STORAGE_META_DATA whether or not there are rows. Nothing compared the two until now: MySQL COM_STMT_PREPARE has reported the four columns as well, but MySQL clients use the column definitions that come with the execution. getMetaData() now returns the header doRun answers with for each form, so Prepare advertises seven columns for the plain form and four for USING and USING FOR. I also went through every ShowCommand subclass looking for the same mismatch, a getMetaData() that differs from the header built by doRun or by the manager it delegates to, and found no other. The headers that depend on more than the syntax are the ones this PR already resolves (HELP, SHOW SNAPSHOT, SHOW CREATE TABLE, SHOW PROC) or keeps on the placeholder. SchemaOnlyCommandMetadataTest#storagePolicyHeadersMatchExecution compares the header of doRun with getMetaData() for all three forms and fails without the fix. test_schema_only now also executes its side-effect-free frontend-side statements as prepared statements, SHOW STORAGE POLICY among them, and compares the executed schema with the advertised one. Until now it only compared Prepare with GetSchema, and both come from the same analysis.
| @Override | ||
| public ShowResultSetMetaData getResultSetMetaData() { | ||
| // Unlike ordinary INSERT, warmup returns per-backend statistics rather than OK. | ||
| return metaData; |
There was a problem hiding this comment.
[P2] Keep empty warmups consistent with the advertised header
With file caching enabled, WARM UP SELECT id FROM t WHERE FALSE on an ordinary table optimizes to BlackholeSink(EmptyRelation). This sets emptyInsert=true, so requiresTransaction() is false and InsertIntoTableCommand.runInternal() returns before registering the warmup statistics listener. Execution succeeds without a result set, and Flight synthesizes the one-column StatusResult. Advertising these six columns makes the new prepared-execution comparison reject that successful empty warmup with NOT_FOUND/schema changed. Emit a statistics result on the empty success path, or retain placeholder preparation until execution guarantees this header; cover the prepared execution of a false-predicate warmup.
There was a problem hiding this comment.
Fixed in 7c7393c8c18. Confirmed, and the case is wider than WHERE FALSE. Planning replaces the source with an empty relation whenever it proves the source empty: a predicate that folds to false, partition pruning that leaves no partition, or selected partitions without data (PruneEmptyPartition). Warming up a table that has no data yet is enough. The sink is then BlackholeSink(EmptyRelation) and requiresTransaction() is false. InsertIntoTableCommand.runInternal returns before the statistics listener is registered, and Flight answers its synthesized StatusResult. So the result header of WARM UP SELECT depends on what planning finds, which is the kind of statement this PR already keeps on the placeholder. WARM UP SELECT now joins that list in FlightSqlQuerySchema.commandMetadata: Prepare advertises the placeholder, its executions are not compared, and GetSchema answers UNIMPLEMENTED, as on master before this PR. The getResultSetMetaData() override is removed, so MySQL COM_STMT_PREPARE describes the statement as it does on master. Another fix would have been to make the command answer its statistics, with an all-zero TOTAL row, on the empty path as well. That changes what MySQL clients get for an empty warmup, so it is not part of this PR. DorisFlightSqlSchemaTest now lists WARM UP SELECT among the placeholder statements. warmupSelectRunsUncheckedWhateverItsSourceHolds runs one prepared warmup whose execution answers OK and one that answers statistics; both checks fail if WARM UP SELECT gets an advertised header again. test_schema_only prepares a WARM UP SELECT on an empty table and, in cloud mode (the only mode that warms up internal tables), runs it.
TPC-H: Total hot run time: 28283 ms |
TPC-DS: Total hot run time: 150494 ms |
ClickBench: Total hot run time: 24.27 s |
…es, SHOW STORAGE POLICY and WARM UP SELECT ### What problem does this PR solve? Issue Number: None Related PR: apache#68631 Problem Summary: Review of this port found three statements that run over the Arrow Flight SQL JDBC driver on master but failed once Prepare started to analyze them. The driver prepares every statement and does not fall back when Prepare refuses one. 1. A CTE used once, under column-level privileges. Prepare checked privileges on the analyzed plan. There the CTE is still a `LogicalCTEAnchor` over its producer, and `ColumnPruning` treats every producer output as needed. So with only `SELECT_PRIV(id)` on `t(id, secret)`, `WITH c AS (SELECT id, secret FROM t) SELECT id FROM c` failed Prepare with INVALID_ARGUMENT. Execution runs the query, because it inlines the CTE first and then checks only the columns read. Prepare now runs `PullUpCteAnchor` and `CTEInline` on the analyzed plan before `CheckPrivileges`, as the rewriter does. The rewriter normalizes the plan further before its own check; for example, it drops an ORDER BY under a derived table. So a denial on the analyzed plan does not show that execution is denied. Such a statement now keeps the placeholder schema and runs unchecked, so execution decides as it did before this PR, and GetSchema answers UNIMPLEMENTED without a schema. 2. SHOW STORAGE POLICY. `ShowStoragePolicyCommand.getMetaData()` returned the four-column USING header for every form, but the plain form returns the seven columns of `StoragePolicy.STORAGE_META_DATA`. Every prepared execution therefore failed with NOT_FOUND "Prepared statement schema changed". It now returns the header that `doRun` uses for each form. This also corrects the column definitions that MySQL `COM_STMT_PREPARE` reports for the statement. 3. WARM UP SELECT. When planning proves the source empty (a predicate that folds to false, no partition left after pruning, or partitions without data), the statement returns OK instead of its statistics, and its prepared execution failed as a schema change. Its header depends on what planning finds, so WARM UP SELECT now keeps the placeholder schema like the other statements whose result depends on execution. The `getResultSetMetaData()` override added for it is removed. The regression suite `test_schema_only` now also executes its side-effect-free frontend-side statements as prepared statements, compares the executed schema with the advertised one, and runs a WARM UP SELECT on an empty table. ### Release note Flight SQL Prepare keeps the placeholder schema for WARM UP SELECT and for queries whose privilege check fails; those queries still fail when they run. The prepared metadata of SHOW STORAGE POLICY lists the seven columns it returns. ### Check List (For Author) - Test: Unit Test, Regression test - `DorisFlightSqlSchemaTest`: new `columnGrantsAreCheckedAsExecutionChecksThem` and `warmupSelectRunsUncheckedWhateverItsSourceHolds`; updated `schemaDiscoveryChecksTablePrivileges` and `commandsWithUnknownOrDynamicMetadataKeepThePlaceholder` - `SchemaOnlyCommandMetadataTest`: new `storagePolicyHeadersMatchExecution` - `arrow_flight_sql_p0/test_schema_only`: prepared executions of frontend-side commands, and WARM UP SELECT on an empty table (its execution only in cloud mode, the only mode that warms up internal tables) - 41 FE test classes (536 tests) pass and fe-core Checkstyle is clean; skipping the inlining, failing Prepare on a denial again, returning the USING header for every form and validating WARM UP SELECT each turn their tests red - Local cluster: all 21 suites under `arrow_flight_sql_p0` and `test_uuid_arrow_flight` pass; the strengthened `test_schema_only` fails on the previous head at the SHOW STORAGE POLICY header - Behavior changed: Yes. A query whose privilege check on the analyzed plan fails now keeps the placeholder at Prepare and fails at execution. WARM UP SELECT keeps the placeholder. SHOW STORAGE POLICY reports its seven columns. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Addressed review round 1 in
New and changed tests, each red without its fix:
41 related FE test classes, 536 tests, pass with Checkstyle. |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed PR #69867 at 7c7393c8c18d62fa9afd2b20405ce24c96c1a5fd. One new P2 correctness finding: untyped NULL result fields advertise Arrow Null while BE sends nullable Bool, so the new comparison rejects an unchanged valid prepared query. The inline comment includes the concrete SQL, conversion chain and fix direction.
The review converged after 2 rounds. All three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS in round 2; all 21 changed files were covered, and all candidates and initial risks have verified dispositions. No unresolved review candidate remains.
The existing findings are fixed at this head: 4419077176 (single-use CTE column pruning) now inlines CTEs before privilege checks and keeps the placeholder for remaining analyzed-plan denials; 4419077184 (ordinary SHOW STORAGE POLICY) now selects the same seven-column header as execution; 4419077191 (empty warmups) now retains the placeholder and skips the execution-schema comparison. These were independently revalidated and are not reposted. There is no existing P0/P1 finding to carry forward. No additional user-provided review focus was supplied.
Critical-checkpoint conclusions:
- Goal and proof: Prepare/GetSchema now derive metadata without dispatching queries, and checked execution compares the advertised and actual schema before publishing tickets. The NULL representation mismatch means this goal is incomplete for that query family. Inspected unit and regression tests exercise the main flows, but miss untyped NULL execution.
- Scope, clarity and reuse: The implementation reuses PrepareCommandPlanner, command metadata, the shared Arrow mapper and FlightSqlSchemaHelper. The Env helper extracts existing provider-default behavior; the timeout conversion is a small, explicit overflow check. No further abstraction or scope defect was established.
- Concurrency: RPC/worker entrypoints serialize mutable prepared queries, session state, channel results and endpoints through the existing command lock. Deferred executors use their separate monitor; cleanup runs outside it. Existing sorted table locking and planner cleanup are preserved. No new deadlock or unsynchronized access was established.
- Lifecycle and initialization: Analysis deep-clones session variables, restores borrowed state and namespace in finally, and explicitly closes statement/connector scopes. ExecutedQuery retains the executor before processor close; mismatch cancellation precedes deferred-result cleanup. The new static placeholder schema has no identified initialization cycle. No additional resource-lifetime issue was found.
- Configuration: No configuration item is added; existing session settings are used and reanalysis detects schema-changing settings.
- Compatibility: Dynamic/unknown schemas retain placeholder preparation for Flight JDBC. TIMEV2/VARBINARY match existing BE wire types. No storage-format or persistent-symbol migration is introduced; M1 is a new query compatibility regression.
- Thrift/protobuf: No definition or field/enum ID changes; release-branch ID comparison is not applicable.
- Parallel paths: Inspected statement/prepared GetSchema and execution, local command/BE result paths, nested fields, canonical time zones, empty/folded/union/subquery outputs and aggregate-state handling. Flight already disables incompatible MySQL cache, FE SELECT and point-query shortcuts. No additional independent defect was verified.
- Conditional/error handling: Unsupported shapes deliberately use placeholders at Prepare and UNIMPLEMENTED at GetSchema; invalid SQL, stale handles, invalid filters and timeout overflow have explicit failure paths. The NULL_TYPE exception to the Arrow Null rejection is the inline finding.
- Test coverage: Inspected positive and negative schema/handle/namespace tests, column grants and CTE shapes, synchronization with latches, allocator checks, mismatch cancellation, command-header-versus-execution checks, and the new Flight regression suite. Bare NULL and nested untyped NULL require real prepared-execution coverage; typed NULL casts do not cover them.
- Expected results: No golden .out file is added or edited. The new suite checks protocol metadata and state directly. No further incorrect expected value was identified by source inspection.
- Observability: Client errors identify invalid handles/schema changes and existing producer/cancellation logs expose failures. No additional required metric or logging gap was established for these metadata paths.
- Transactions and persistence: No EditLog/replay, commit/visibility, failover or delete-bitmap behavior is changed; these checkpoints are not applicable.
- Data writes and crashes: Discovery does not execute DML/DDL; existing execution remains responsible for transactionality. No new write atomicity or recovery path is introduced, and mismatch cleanup is covered by the lifecycle review.
- FE/BE variables: No new cross-process variable or sending point is added; the time zone uses the existing canonical query-global value.
- Performance: Extra FE analysis at Prepare/GetSchema and checked prepared execution is the stated feature cost. No additional demonstrated hot-path performance defect was found; no benchmark was run.
- Other issues: Rechecked inherited commands, conditional headers, USE/SWITCH remembered/default database behavior and timeout callers. No further independent finding was substantiated.
Validation limits: static source and test inspection only. No builds, tests, benchmarks, runtime queries or external-connector validation were performed; reported author test results were not treated as locally verified results.
Review converged after 2 round(s).
| // BE stamps the session time zone, where the shared mapping still says "UTC" (#67577). | ||
| arrowType = new ArrowType.Timestamp(((ArrowType.Timestamp) arrowType).getUnit(), timezone); | ||
| } | ||
| if (arrowType instanceof ArrowType.Null && primitive != PrimitiveType.NULL_TYPE) { |
There was a problem hiding this comment.
[P2] Match BE's wire type for untyped NULL outputs
SELECT NULL AS n reaches this branch as NULL_TYPE, so Prepare advertises Arrow Null. BE's DataTypeFactory represents TYPE_NULL as nullable DataTypeUInt8 (TYPE_BOOLEAN), and arrow_row_batch.cpp therefore emits Arrow Bool. The new matchesExecutionField comparison rejects that unchanged query with NOT_FOUND / "schema changed" and removes its handle. Flight JDBC prepares every statement, so this breaks an otherwise valid query; nested untyped NULL fields share the mismatch. Resolve NULL_TYPE to the same wire representation recursively, or retain the placeholder until supported, and cover prepared execution of an untyped NULL. The new nullable regression case uses CAST(NULL AS VARCHAR), which avoids this path.
There was a problem hiding this comment.
Fixed in 36772dc. Confirmed: data_type_factory.cpp builds an untyped NULL column as a DataTypeUInt8 flagged as a NULL literal, whose primitive type is BOOLEAN, so convert_to_arrow_type sends nullable Bool and the TYPE_NULL -> arrow::null() branch is never reached for it. The advertised Null therefore failed the comparison after every prepared execution; query_p0/operator/test_sort_operator (select org_id,null from dim_org ...) failed this way over the Arrow Flight SQL JDBC driver in P0, and fails the same way on a local cluster with the previous head.
arrowField already keeps the placeholder for every result type that maps to Arrow Null; NULL_TYPE was the one exception, and now it is not. A result with an untyped NULL column, at any depth (arrowField recurses into ARRAY, MAP and STRUCT), keeps the placeholder schema on Prepare, its executions are not checked, and GetSchema answers UNIMPLEMENTED, which is what master does without this port. A typed NULL such as CAST(NULL AS VARCHAR(20)) is still described as its type. I did not advertise Bool instead: that would make the frontend depend on how BE happens to represent a NULL literal, and an untyped NULL column carries no type a client could use.
Tests: DorisFlightSqlSchemaTest#untypedNullResultsKeepThePlaceholder covers SELECT NULL, SELECT id, NULL AS n FROM ... ORDER BY id, and NULL inside ARRAY, MAP and STRUCT, and fails with the previous condition. test_schema_only adds SELECT NULL and SELECT ARRAY(NULL) to the placeholder statements and runs a prepared SELECT k, NULL AS n ... twice through the backends. On a local cluster, test_sort_operator over Flight SQL fails with the previous head and passes with this one.
FE Regression Coverage ReportIncrement line coverage |
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28649 ms |
TPC-DS: Total hot run time: 151146 ms |
ClickBench: Total hot run time: 24.23 s |
… schema ### What problem does this PR solve? Issue Number: None Related PR: apache#68631 Problem Summary: Prepare advertised an untyped NULL result column (NULL_TYPE) as Arrow `Null`, the type `DorisArrowTypeMapping` gives NULL_TYPE. BE never sends that: `data_type_factory.cpp` builds the column as a `DataTypeUInt8` flagged as a NULL literal, whose primitive type is BOOLEAN, so `convert_to_arrow_type` sends nullable `Bool`. The comparison after a prepared execution therefore failed, removed the handle and answered NOT_FOUND "Prepared statement schema changed; prepare again" for every execution of such a query. Over the Arrow Flight SQL JDBC driver, which prepares every statement, `select org_id, null from dim_org ...` in `query_p0/operator/test_sort_operator` failed this way. Before apache#69867 the query ran, because Prepare always returned the placeholder schema. `FlightSqlQuerySchema.arrowField` already treats a result type that maps to Arrow `Null` as one this frontend cannot describe as BE sends it, and keeps the placeholder for it, except for NULL_TYPE. It now covers NULL_TYPE too, at any depth since `arrowField` recurses into ARRAY, MAP and STRUCT children: Prepare returns the placeholder, the executions are not checked, and GetSchema answers UNIMPLEMENTED, as on master before apache#69867. A NULL with a type (`CAST(NULL AS VARCHAR(20))`) is still described as that type. Advertising `Bool` instead would tie the frontend to how BE happens to store a NULL literal. ### Release note Flight SQL Prepare keeps the placeholder schema for a result with an untyped NULL column, so its prepared executions run instead of failing with "Prepared statement schema changed". ### Check List (For Author) - Test: Unit Test, Regression test - `DorisFlightSqlSchemaTest#untypedNullResultsKeepThePlaceholder`: `SELECT NULL`, `SELECT id, NULL ... ORDER BY id`, and NULL inside ARRAY, MAP and STRUCT keep the placeholder, their handles carry no schema to check, and GetSchema answers UNIMPLEMENTED. It fails with the previous condition restored. - `arrow_flight_sql_p0/test_schema_only` adds `SELECT NULL` and `SELECT ARRAY(NULL)` to the placeholder statements and runs a prepared `SELECT k, NULL AS n ...` twice through the backends. - 41 FE test classes (537 tests) pass and fe-core Checkstyle is clean. - Local cluster: `test_sort_operator` over Flight SQL fails with "Prepared statement schema changed" on the previous head and passes on this one; all 20 suites under `arrow_flight_sql_p0` and `test_uuid_arrow_flight` pass; the strengthened `test_schema_only` fails on the previous head at `SELECT NULL AS n`. - Behavior changed: Yes. A result with an untyped NULL column keeps the placeholder schema on Prepare, as master does without this port. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
run buildall |
TPC-H: Total hot run time: 28548 ms |
TPC-DS: Total hot run time: 151716 ms |
ClickBench: Total hot run time: 24.13 s |
|
Addressed review round 2 in
New and changed tests, each red without the fix:
41 related FE test classes, 537 tests, pass with Checkstyle. On a local cluster, all |
|
/review |
There was a problem hiding this comment.
Reviewed apache/doris PR 69867 at exact head 36772dc against base 2a4191c. The change adds FE-side Flight SQL schema discovery and prepared-execution schema checks, with BE Arrow type alignment and command metadata coverage. I found one new P2 lifecycle issue described inline: a schema-known prepared GetFlightInfo can fail during schema/namespace revalidation before beginRequest clears the prior Flight request, leaving a deferred coordinator or cached result until the idle cleanup bound. The four existing P2 threads (CTE privilege pruning, SHOW STORAGE POLICY, empty WARM UP SELECT, and untyped NULL) were independently revalidated as fixed at this head and are not reposted. No existing P0/P1 finding still applies.
Critical checkpoints: the analyzer restores session variables, state, executor, namespace, and analysis resources; command locking and deferred-executor ownership are serialized; FE Arrow mapping matches the BE wire types for supported scalar and nested fields and preserves placeholders for unsupported representations; command headers match execution forms; and the Env/timeout refactors have no additional issue. The user focus file has no extra focus. This was a static review only; builds and tests were inspected but not executed. Convergence was reached after 2 bounded rounds.
Review converged after 2 round(s).
| return FlightProtocolAdapter.of(connectContext).callCommand(connectContext, | ||
| () -> executeQueryStatement(executedPeerIdentity, connectContext, | ||
| connectContext.getPreparedQuery(preparedStatementId), descriptor)); | ||
| () -> executePreparedStatement(context.peerIdentity(), connectContext, preparedStatementId, |
There was a problem hiding this comment.
[P2] Clear the previous request before a prepared schema precheck can fail
getFlightInfoPreparedStatement runs checkPreparedSchema() before executeQueryStatement() reaches FlightProtocolAdapter.beginRequest(). If the schema is stale (for example after a sql_mode change or DDL), checkPreparedSchema removes the handle and throws NOT_FOUND, so the prior request's deferred coordinator, endpoints, and cached frontend result are never reset. A client that executes a remote query, then makes this failed prepared request, leaves that coordinator pinned until the idle timeout instead of following the normal query cleanup path. Start the request cleanup before this precheck, or explicitly perform the same cleanup on every precheck failure.
There was a problem hiding this comment.
Fixed in be929ac. Confirmed: executePreparedStatement ran preparedQuery() and checkPreparedSchema() before executeQueryStatement() reached FlightProtocolAdapter.beginRequest(), and getFlightInfoPreparedStatement has no catch of its own. A handle refused there (missing, prepared in another namespace, a changed schema, a failed re-analysis) therefore left the previous request's deferred coordinator, its frontend-side result and its endpoints in place. Without this PR, every prepared GetFlightInfo reached executeQueryStatement(); even a missing handle failed inside its try, and the catch cancelled and finalized the previous request's deferred coordinator.
executePreparedStatement now calls beginRequest() before the checks. A prepared GetFlightInfo is the session's next request whatever the checks find, and the call in executeQueryStatement() then finds nothing left. The call is not in the command lock (runCommand / callCommand) or in preparedQuery(): Prepare, GetSchema and the other metadata requests go through those too, and they must leave the previous request alone (FlightProtocolAdapter's class comment, FlightSessionOptionsTest#testTheOptionsLeaveTheLastRequestAlone).
Two parts of the description go further than what happens:
- The state is not kept until the idle timeout whatever comes next. The session's next statement, or its next prepared execution that passes the checks, drops it, and so does session teardown. Only when neither arrives does the timeout checker finalize the deferred coordinator, and its bound is
arrow_flight_deferred_query_idle_timeout_second(3600 s) or the query's own execution timeout, whichever is longer, counted from that query's start, notwait_timeout. - Only a query whose coordinator must outlive GetFlightInfo, a batch-mode external-table scan or a remote Doris scan (
coord.mustOutliveDispatch()), leaves a deferred coordinator; every other query closes its coordinator before GetFlightInfo returns. As for thesql_modeexample:SET sql_mode = ...is itself a GetFlightInfo, which already dropped the previous query's coordinator and leaves only its own OK result. For a schema change to leave a coordinator behind, it has to come from outside the session's requests, such as SetSessionOptions or another session's DDL or REVOKE.
The same commit expires a checked handle whose statement can no longer be described when it is re-analyzed, for example after a column grant was revoked so that the analyzed plan is denied, or after a DDL that turned a result column into an untyped NULL. Such a handle answered UNIMPLEMENTED and was kept, so every later execution failed the same way. The check now compares the schema Prepare would advertise now, which for such a statement is the placeholder: the handle is removed, the request answers NOT_FOUND "Prepared statement schema changed; prepare again", and the statement prepared again keeps the placeholder and runs unchecked.
Tests: DorisFlightSqlSchemaTest adds one test for each way the checks refuse a handle: a missing handle, a namespace change, a sql_mode drift, a dropped view (INVALID_ARGUMENT, the handle is kept) and a revoked column grant (NOT_FOUND, the handle is removed, and preparing again gives the placeholder). Each sends GetSchema and GetFlightInfo after a previous request left a deferred executor, a frontend-side result and an endpoint, and checks the status, whether the handle is kept, and that GetFlightInfo finalized and dropped all three while GetSchema left them. Without the new beginRequest() call, the five tests fail at GetFlightInfo; with the call in preparedQuery() instead, they fail at GetSchema; with the previous UNIMPLEMENTED for a re-analysis that cannot describe the result, only the revoked-grant test fails. branch-4.1 and branch-4.2 check the handle in the same order.
…red handle ### What problem does this PR solve? Issue Number: None Related PR: apache#68631 Problem Summary: A prepared GetFlightInfo checks its handle before it executes anything: the handle must exist, the session must still be in the namespace it was prepared in, and the statement re-analyzed now must have the schema that Prepare advertised. `executePreparedStatement` ran these checks before `executeQueryStatement` reached `FlightProtocolAdapter.beginRequest()`, where a request drops what the previous one left: its deferred coordinator, the frontend-side result no DoGet took, and its endpoints. A handle that the checks refused therefore left all three in place. The previous query's deferred coordinator, which a batch-mode external-table scan or a remote Doris scan leaves, kept its workload group queue slot and its query registration until the session's next statement, the timeout checker or teardown, and the previous frontend-side result could still be fetched. Before apache#69867, every prepared GetFlightInfo reached `executeQueryStatement`, and a missing handle failed inside it, where the catch finalized the previous coordinator. `executePreparedStatement` now calls `beginRequest()` before the checks: a prepared execution is the session's next request whatever they find. Prepare and GetSchema are metadata requests and still leave the previous request alone, so the call is neither in the session's command lock nor in `preparedQuery()`, which GetSchema goes through too. A checked handle whose statement could no longer be described when it was re-analyzed answered UNIMPLEMENTED and was kept, so every later execution failed the same way. That happens after a column grant is revoked so that the analyzed plan is denied, or after a DDL that turns a result column into an untyped NULL. The check now compares the schema Prepare would advertise now, the placeholder for such a statement: the handle is removed and the request answers NOT_FOUND "Prepared statement schema changed; prepare again", as for any other schema change, and the statement prepared again keeps the placeholder and runs unchecked. Prepare and the check share `advertisedSchema` for this. ### Release note A Flight SQL prepared execution refused before it runs no longer keeps the previous query's deferred coordinator and result. A prepared statement whose result schema can no longer be determined before it runs answers NOT_FOUND and must be prepared again. ### Check List (For Author) - Test: Unit Test, Regression test - `DorisFlightSqlSchemaTest` adds one test for each way the checks refuse a handle: a missing handle, a namespace change, a sql_mode drift, a dropped view (INVALID_ARGUMENT, the handle is kept) and a revoked column grant (NOT_FOUND, the handle is removed, and preparing again gives the placeholder). Each sends GetSchema and GetFlightInfo after a previous request left a deferred executor, a frontend-side result and an endpoint, and checks that only GetFlightInfo finished it. - Without the new `beginRequest()` call the five tests fail at GetFlightInfo; with the call in `preparedQuery()` they fail at GetSchema; with the previous UNIMPLEMENTED only the revoked-grant test fails. - 45 FE test classes (555 tests) pass and fe-core Checkstyle is clean. - Local cluster: all 21 suites under `arrow_flight_sql_p0`, `test_uuid_arrow_flight` and `test_sort_operator` over Flight SQL pass. - Behavior changed: Yes. A prepared GetFlightInfo that its checks refuse finishes the previous request first. A checked handle that can no longer be described is removed and answers NOT_FOUND instead of UNIMPLEMENTED. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
run buildall |
|
Addressed review round 3 in
New tests, each red without its fix:
45 related FE test classes, 555 tests, pass with Checkstyle. On a local cluster, all |
|
/review |
TPC-H: Total hot run time: 28429 ms |
There was a problem hiding this comment.
Round 1 complete and converged for PR 69867 at head be929ac. I reviewed all 21 changed files and the required FE/Flight lifecycle, command metadata, Arrow mapping, catalog, timeout, concurrency, cleanup, error handling, and test-coverage checkpoints. The new schema-only analysis restores session/planner state and closes analysis resources; prepared-handle identity, namespace binding, schema drift checks, local/remote result ownership, and deferred-query cleanup are consistent. Command headers were checked against execution headers, and FE mappings were compared with BE Arrow output including nested fields, logical markers, timestamps, VARIANT, TIMEV2, VARBINARY, and AGG_STATE. The Env and FeServiceClient changes were traced through all call sites and boundary values. No verified new finding remains, so there are no inline comments. Existing P2 threads 4419077176, 4419077184, 4419077191, 4420363330, and 4430799614 were revalidated and their fixes are present on this head; none is an applicable P0/P1 blocking finding. User review focus: none provided. This was a static review; builds and tests were not run.
Review converged after 1 round(s).
TPC-DS: Total hot run time: 150940 ms |
ClickBench: Total hot run time: 24.23 s |
|
run external |
924060929
left a comment
924060929
left a comment
There was a problem hiding this comment.
Flight SQL clients need result metadata before execution. This PR replaces placeholder-only preparation with FE analysis, implements GetSchema, and validates prepared results before publishing tickets while preserving placeholder fallback for undiscoverable schemas.
I independently reviewed the specified head against merge-base 2a4191c. No actionable P0/P1/P2 issue remains within the FE scope and necessary BE Arrow boundaries. The review covered analysis-stage ownership, command headers, nested types and nullability, namespace/settings drift, command serialization, and result cleanup. All five previous P2 findings were rechecked against current code and are fixed. Recommendation: APPROVE.
Gabriel39's approval targets this same head. The supplied snapshot reports successful P0/cloud P0, FE UT, compilation and AI review, but failed FE coverage. Required-check rules were not supplied, so this is not a merge-readiness determination. Static review only; no builds or tests were run.
Automated FE-focused static review (gpt-6.1-sol, high). No build or tests were run.
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
…hemas ### Background ConnectContext coordinates client admission and resource lifetime. Flight prepared statements now validate execution response schemas against the schema advertised during Prepare. ### What problem does this PR solve? Issue Number: None Related PR: apache#70064, apache#69867 PR Type: Refactor Problem Summary: Integrate upstream/master while retaining deferred session cleanup and self-KILL responses. A self KILL CONNECTION response must carry the advertised StatusResult schema while avoiding a DoGet ticket for its revoked session. Also snapshot the volatile executor when reading its timeout to avoid racing the previous command's clear(). ### Details - **Change map:** Integrate upstream/master at 897bb25; resolve the Flight producer response conflict, add prepared self-KILL tests, adapt the new schema fixture to admission/cleanup, and use one local executor snapshot for timeout calculation. The other upstream changes are retained as-is. - **Moves vs. logic changes:** Retain current Flight schema handling and the unified reference-based cleanup; represent self-KILL with its matching status schema and no ticket. - **Behavior change:** Prepared self KILL CONNECTION succeeds without a schema mismatch; self KILL QUERY retains its session and prepared handle. - **Compatibility:** No wire definitions, stored formats or configuration changes. The response header matches Prepare. - **Performance impact:** No new shared state, lock or command-path allocation for timeout calculation. - **Rollback:** Revert the lifecycle PR as a unit; no metadata migration is required. ### Testing - ./run-fe-ut.sh --run with 17 selected lifecycle, protocol, timeout, KILL and schema test classes: 272 tests passed, zero failures/errors/skips; full script BUILD SUCCESS. - mvn -f fe/pom.xml checkstyle:check -pl fe-core: passed. - git diff --cached upstream/master --check: passed. - Verified report totals and source/compiled-class timestamps. No MySQL golden changes. - Cluster regression tests were not run. ### Release note None ### Check List (For Author) - Behavior changed: Yes (see Details). - Does this need documentation: Yes; ownership and caller responsibilities are documented in the English class comments. - Pre-submission status: Partially verified: selected unit tests and style checks passed; cluster regression was not run. ### AI Disclosure - Tool: OpenAI Codex (GPT-6). - Generated: Conflict resolution, tests and this description. - Human review: Pending.
What problem does this PR solve?
Issue Number: None
Related PR: #68631, #68768
Problem Summary:
In short. This ports branch-4.1 #68631 (also on branch-4.2) to master. A Flight SQL
CreatePreparedStatementnow returns the analyzed result schema of the statement,GetSchemais implemented for both statements and prepared statements, and a prepared execution is checked against the schema that Prepare advertised. Before this PR, Prepare always returned a fixed one-column placeholder, andGetSchemaanswered UNIMPLEMENTED. The analysis runs in the FE only and does not plan or execute the query.Original author: @Gabriel39
Background. Flight SQL clients learn the shape of a result before they fetch it. The Arrow Flight SQL JDBC driver prepares every statement and builds
ResultSetMetaDatafrom the dataset schema thatCreatePreparedStatementreturns. ADBC exposes the same schema asexecute_schema, andGetSchemais the RPC that asks for it without running anything.The problem, and what it cost. On master:
CreatePreparedStatementdataset schemaResultMeta: Utf8, whatever the statementGetSchemafor a statement or a prepared statementNull, while BE sendsfloat64/binaryFloat64/Binary, as BE sends themA client that trusts the advertised schema saw one string column for every query, and could not ask for a schema at all without executing.
How this PR fixes it.
arrowflight/FlightSqlQuerySchemaanalyzes the statement on a clone of the session variables: a USE / SWITCH prefix is resolved in a temporary namespace, the analysisStatementContextis closed on the same thread, and the connector statement scope it may have opened is closed explicitly. Each field gets the samedoris_typemarkers and native Variant layout the execution side uses (FlightSqlSchemaHelper.withDorisTypeMetadata/nativeVariantField), and TIMESTAMPTZ carries the canonical session time zone that BE stamps on its batches.PullUpCteAnchor,CTEInline, as the rewriter does before its own check). A CTE used once is therefore checked only on the columns the query reads, and a CTE used more than once on every column it selects, which is also how execution checks them. The rewriter normalizes the plan further before it checks privileges. For example, it drops an ORDER BY under a derived table, so the analyzed plan can read a column that the executed plan does not. When the analyzed plan is denied, the statement keeps the placeholder and execution decides. No schema is disclosed in either case.doRun, including master's stream, plugin-view and system-table cases), SHOW PROC/and/current_queries, SHOW CONFIG, and EXPLAIN PLAN PROCESS (Rule / Before / After).ShowStoragePolicyCommand.getMetaData()returned the four-column USING header for every form, while plain SHOW STORAGE POLICY answers the seven columns ofStoragePolicy.STORAGE_META_DATA. It now returns the header of the form that runs. This also corrects the column definitions that MySQLCOM_STMT_PREPAREreports for the statement. No other SHOW command declares a header that differs from the one it returns.SELECT NULL,SELECT id, NULL FROM t,ARRAY(NULL)).DorisArrowTypeMappinggives NULL_TYPE ArrowNull, but BE builds such a column as aUInt8flagged as a NULL literal and sends it as nullableBool, so an advertisedNullwould expire the handle at every execution. A typed NULL (CAST(NULL AS VARCHAR(20))) is described as its type.FlightProtocolAdapterrecords the SQL, the namespace and the advertised schema. A prepared execution re-analyzes the statement and compares the result with the advertised schema before it runs, and compares the schema it actually produced before it publishes a DoGet ticket. It is the session's next request whether or not these checks pass: before them, it drops what the previous request left, as a statement's execution does, while Prepare and GetSchema are metadata requests and leave it. On a mismatch, the handle is removed, the execution is cancelled, and the deferred coordinator, the channel result and the endpoints are dropped. A handle whose statement can no longer be described when it is re-analyzed, for example after a column grant was revoked, expires the same way, and the statement prepared again keeps the placeholder. A USE / SWITCH inside the statement rebinds only that handle.DorisArrowTypeMappingmaps TIMEV2 toFloat64and VARBINARY toBinary, the types BE sends. TIMESTAMPTZ keeps the fixed"UTC"in GetTables, and AGG_STATE keepsNullthere.FeServiceClientrejects a remote Doris metadata timeout whose milliseconds do not fit in an int, instead of letting it wrap around.Where master differs from branch-4.x:
?placeholders, and result types the FE cannot describe. Python ADBC falls back to executing without a prepared statement, but the Arrow Flight SQL JDBC driver prepares every statement and has no fallback, so on master these statements would fail over JDBC.test_arrow_flight_session_lifecycleruns DESC andSET @multi = 2; SELECT ...through JDBC. Here, these statements keep today's placeholder schema, and their handles are not checked at execution.Nullfor an untyped NULL column, which BE sends asBool, so every prepared execution of such a query is rejected as a schema change. Here, such a result keeps the placeholder.FlightProtocolAdapter.runCommand/callCommand) instead ofsynchronized (connectContext), and the namespace is saved and restored withgetNameSpaceContext()/restoreNameSpaceContext().Env.defaultDatabaseOnUse, now shared withEnv.changeCatalog) instead ofEsExternalCatalog.DEFAULT_DB.GetTableTypes, the other half of [fix](flight-sql) Return analyzed query schemas and table type metadata #68631, is implemented by [fix](arrow-flight) Support table type metadata and filters #67899.Cost: every Prepare, every prepared execution and every
GetSchemaruns one more FE analysis of the statement.Results. On a local cluster built from this branch, every suite under
arrow_flight_sql_p0passes: the newtest_schema_onlyandtest_get_tables_schema, plus the seven suites that run through Flight JDBC,test_arrow_flight_session_lifecycleamong them.datatype_p0/uuid/test_uuid_arrow_flightpasses too.test_schema_onlychecks the advertised schema of each kind of statement against what its execution actually returns. That covers queries, the side-effect-free frontend-side commands executed as prepared statements (SHOW STORAGE POLICY among them), a WARM UP SELECT on an empty table (executed in cloud mode, the only mode that warms up internal tables), and agroup_concat_stateAGG_STATE, which BE sends as Utf8.Release note
Flight SQL
CreatePreparedStatementandGetSchemareturn the analyzed result schema of a statement. Statements whose schema is only known once they run, and results with an untyped NULL column, keep returning the placeholder schema, and so do queries whose privilege check fails, which still fail when they run. A prepared statement is refused with NOT_FOUND and must be prepared again when the session's catalog or database, or the result schema, changed since it was prepared. Invalid SQL is reported at Prepare. Flight SQL now describes TIMEV2 columns as Float64 and VARBINARY columns as Binary, matching the data it sends. The prepared metadata of SHOW STORAGE POLICY lists the seven columns it returns.Check List (For Author)
arrow_flight_sql_p0/test_schema_only. On a local cluster built from this branch, all suites underarrow_flight_sql_p0anddatatype_p0/uuid/test_uuid_arrow_flightpass, andquery_p0/operator/test_sort_operatorpasses over Flight SQL (-g arrow_flight_sql), where the previous head fails it with "Prepared statement schema changed".external_table_p0/iceberg/test_iceberg_arrow_flight_split_sourceneeds the docker environment and was not run locally.DorisFlightSqlSchemaTest,FlightSqlExecutionSchemaTest,SchemaOnlyCommandMetadataTestandFeServiceClientTest. UpdatedDorisArrowTypeMappingTestandDorisFlightSqlProducerTest; its allocator leak test now uses statements that keep the placeholder. Together with the existing tests of the changed classes (run-fe-ut.shplus fe-core Checkstyle): 45 classes, 555 tests, all pass.Nullfor an untyped NULL column, checking a prepared handle before ending the previous request, ending the previous request on GetSchema too, and keeping a checked handle whose statement can no longer be described.Check List (For Reviewer who merge this PR)