Conversation
|
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.
Static review of the opt-in native Parquet VARIANT Arrow Flight result format at head 3981f52. Five inline findings (three P1, two P2) cover mixed-version transport, legacy value conversion, a legacy depth limit, mixed string roots, and nested timezone binding. The ordinary scalar and V2 nested mapping is present, but the feature does not yet meet its correctness and compatibility goal in these cases.
Part 1.3 checkpoints:
- Goal, scope, and tests: The changes are focused on the session flag, FE/BE sink transport, Arrow schema and writer, and native Variant tests. BE, FE, Groovy, and Python coverage exercises representative default/native scalar and nested paths. The tests do not cover the five reported triggers. No build or test was run under this review contract; fixture and author claims are static evidence only.
- Concurrency and lifecycle: Existing parallel and nonparallel result-sink construction, local/remote Flight readers, and schema publication were traced. No new thread, lock, lock order, shared mutable writer state, or cross-TU static initialization dependency was introduced. Extension registration is lazy and the reader registers it before decoding. No independent lifecycle or deadlock issue was found.
- Configuration and compatibility: The new session option defaults false and is copied into the result sink; both BE sender paths consume the optional thrift bit. Rolling FE/BE and proxy capability is insufficient (P1). The native nested fixed-offset schema binding fails on the same version (P2). No persistent or dynamic global configuration is added.
- Parallel paths and conditions: Parallel schema equality, single-sink DoGet proxying, recursive Array/Map/Struct mapping, nullable/constant columns, SQL nulls, and empty blocks were checked. The legacy scalar shortcut and JSON fallback have the reported exactness, string-family, and depth defects. Other examined conditions had no additional substantiated issue.
- Errors, memory, and performance: Changed status returns and the Arrow exception boundary propagate failures rather than silently continuing, except the reported JSON round-trip can silently change a decimal or string type/value. No independent memory leak was established. Re-serializing every legacy composite row as JSON adds CPU and allocations; the correctness findings take priority over a separate performance comment.
- Observability: Existing Flight conversion statuses and logs identify a failing column; no separate metric or logging defect was established.
- Transactions, persistence, data writes, and visibility: This is a result-encoding change. It adds no transaction, EditLog, stored format write, visible-version, or delete-bitmap path, so no issue was found in those checkpoints.
- FE-to-BE variable transfer: The setting is represented in both the FE sink and optional thrift field and consumed by both BE sender branches; the unsupported mixed-version route is the reported exception.
User focus: no additional focus was supplied. Existing inline threads: none at the refreshed exact head, so there are no existing blocking comment IDs. Review completion status: INCOMPLETE convergence under the mandated three-round limit because a new valuable nested-timezone finding appeared in round three. All currently identified candidates were independently checked and either included below, deduplicated, or dismissed with evidence in the shared ledger; the five substantiated findings are submitted now. This opinion is static-only.
|
run buildall |
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Complete static review of PR #68667 at head ff92c38643d47fe3234dcf17063b1436614c33ac. I found two new P1 issues in legacy VARIANT native conversion. Existing P1 inline comment 4141016458 remains applicable when BE membership or version changes after the result sink captures its capability decision; I did not repeat it. The other existing P1 examples (mixed string-family roots and decimal/DATE roots) are addressed on this head. The existing depth-limit and nested-timezone P2 threads have their documented limit or code fix. No additional user review focus was supplied.
Critical checkpoints (skill Part 1.3):
- Goal and proof: The opt-in session flag, optional heartbeat and sink fields, recursive Arrow extension schema, BE writer, remote IPC registration, GetTables metadata, and client examples implement native Parquet Variant output for supported values. The added BE, FE, Groovy and Python cases cover basic values, nested schema, SQL NULL, parallel sinks, downgrade gating, and partition reads. They do not prove full legacy compatibility: the two new inline findings identify legal values that fail or change meaning. This was a static review only; I ran no builds or tests and do not treat the author's test report as independent validation.
- Scope and focus: The change is centered on Flight native VARIANT. The adapter's V2 CAST dispatch is too broad for the legal legacy source families, and its scalar shortcut skips the legacy root-visibility rule. The proposed comments point to those exact branches. All 25 changed files were swept after convergence; no separate issue remained unresolved.
- Concurrency: Heartbeat threads update a volatile BE capability while planning reads backend state and freezes the format in
ResultSink. Backend selection and proxy ticket use occur later. A newly joined or downgraded older BE can therefore still encounter a frozen native result, as covered by existing P1 comment 4141016458. No new lock order, blocking operation under a lock, or independent thread-safety defect was found. - Lifecycle and initialization: Sink format is query-scoped; local and remote Flight readers use the published schema, and remote extension registration precedes IPC decoding. The extension registration is function-local static and introduces no observed cross-translation-unit initializer dependency or ownership cycle. The ticket can outlive the capability snapshot, as noted above.
- Configuration and compatibility:
enable_arrow_flight_sql_native_variantdefaults off and is captured for the query; a missing optional BE heartbeat bit defaults false, with successful heartbeat/replay and downgrade handling. Both Thrift additions are optional. The remaining mixed-version timing window is the existing P1. The documented native depth and Decimal256 limits were checked against the old threads; other legal legacy types still fail as described inline. - Parallel paths and conditions: Both ordinary and parallel result-sink schema creation pass the frozen flag. Recursive ARRAY/MAP/STRUCT extension binding, fixed-offset timestamp aliases, FE GetTables metadata, and local/remote Flight paths were traced. The existing nested-timezone issue is fixed here. The scalar-root condition and the unconditional visible-root V2 CAST are the two failing conditions; no further parallel-path issue was substantiated.
- Errors, tests, and observability: New Arrow and CAST errors generally propagate as non-OK status; the unsupported legacy families now fail the query rather than silently converting. The depth error names the UTF8 fallback. Tests assert deterministic basic output but omit the composite/TIMEV2 and scalar-null cases; no
.outresult was changed. Existing logging and error context were adequate for the reviewed paths, and no additional metric need was established. - Persistence, writes, and versions: This PR changes result serialization and BE capability state, not table writes, transaction commit, visible-version, or delete-bitmap logic. The capability is included in heartbeat replay/image state; no independent EditLog or crash-recovery issue was found.
- FE-to-BE state, memory, and performance: The result-format bit is carried in
TResultSink, and BE schema creation consumes it in both sink modes; the heartbeat bit is carried back to FE. The conversion uses per-block masks and encoded batches. Static tracing found no independent memory-lifetime or performance defect beyond the unsupported dispatch and changed null value. No runtime benchmark or memory test was run.
Review completion: Two review rounds were completed. Round 2 normal and focused reviewers all returned NO_NEW_VALUABLE_FINDINGS; every candidate was verified, merged, accepted, or dismissed against existing threads. The final changed-file and unresolved-candidate sweep is complete. User focus: none beyond the full PR review.
Existing P0/P1 findings confirmed for this head: #68667 (comment)
|
run buildall |
|
/review |
There was a problem hiding this comment.
Review conclusion
Changes requested on the current head (474438bfe2df9cbbe47e21ecb173454d38df659c). The existing P1 inline finding 4141922700 remains applicable. I found no distinct new issue that warrants another inline comment.
A SQL expression such as CAST(ARRAY(CAST('42' AS VARBINARY)) AS VARIANT) is admitted by the Nereids ARRAY and ARRAY-to-VARIANT cast checks. The BE legacy cast retains the ARRAY root. With enable_arrow_flight_sql_native_variant=true, append_legacy_arrow_value descends into the VARBINARY element and returns NotSupported; the existing UTF8 SerDe can return the ASCII element as [42]. This is another legal ARRAY leaf under the existing thread's request to cover all legacy roots, so I have listed that comment ID as still blocking instead of posting a duplicate. The added tests do not cover this leaf.
Critical checkpoints
- Goal, scope, and tests: The opt-in native Parquet Variant extension is wired through FE planning, BE schema generation, writers, and Flight readers, with UTF8 as the default. The unsupported legal ARRAY leaf means the full goal is not yet met. The changes are focused on that protocol path. Unit, regression, and sample cases cover scalar and nested values, SQL nulls, slices, timezone aliases, IPC, heartbeat replay, and both result-sink settings; they do not prove this leaf or force multiple parallel endpoints. This review is static only: no build or test was run, and author-reported passes were not treated as independent validation. No changed test-result
.outfile is involved. - Concurrency and lifecycle: The new value builders are query local; extension registration uses a function-local static. The capability flag is volatile and backend-map reads use immutable snapshots. I found no new lock-order, deadlock, ownership-cycle, or cross-translation-unit initialization defect. Result format is captured with the sink, and the buffer schema is reused by schema fetch and local or remote DoGet. A downgrade before the next heartbeat remains within the already reported mixed-version discussion, not a distinct new inline finding.
- Configuration, compatibility, and parallel paths: The session option is dynamic between queries and defaults to false. The new thrift fields are optional and default false, and heartbeat replay carries the capability. Both parallel and nonparallel sink creation pass the captured flag; GetTables recursively describes native nested fields; remote readers register the extension before IPC decoding. Existing null-root, typed-decimal, mixed-string, nested-timezone, and documented depth-limit concerns were checked against this head and were not reconfirmed as blockers.
- Conditions, errors, data, and observability: Root visibility and outer SQL-null masks are preserved by the inspected branches. Unsupported leaves return a non-OK status through the Arrow conversion boundary. The change does not modify transaction commits, storage visibility, delete bitmaps, or write-path atomicity. Existing status and Flight schema errors provide useful failure context. The all-backend capability scan and temporary root/document batches add work, but I found no separately substantiated performance defect or necessary new metric.
User focus: no additional focus was supplied; all 25 changed files and their relevant call paths were reviewed. The only independently confirmed still-applicable P0/P1 inline comment is 4141922700.
Existing P0/P1 findings confirmed for this head: #68667 (comment)
|
run buildall |
|
/review |
|
Fixed the new Cloud P0 failure in Validation: reproduced the original INSERT failure on a dedicated local Doris 4.1 cluster with a subcolumn limit of 3; the updated fixture passed with |
There was a problem hiding this comment.
COMPLETE static review of PR #68667 at bf96e70. I reviewed all 25 changed files, their relevant callers and consumers, and the existing inline threads. Two normal full-review passes plus a separate risk scan converged with NO_NEW_VALUABLE_FINDINGS for distinct inline comments. I request changes because three previously reported P1 concerns remain reachable on this head; I have not reposted them.
Still-applicable existing P1 threads:
- 4141016458 (mixed-version routing): the new heartbeat gate handles a stable old BE, but failed heartbeats leave a previously advertised capability true until a later successful reply. During a rollback, or if an older proxy enters between sink planning and DoGet, a native ticket can still be served by an older BE. This remains within the original rolling-upgrade/proxy concern.
- 4141016465 (typed legacy JSON fallback): visible roots now use typed encoding, but a V1 VARIANT row with a null root and a predefined typed document path still enters the new document branch. It serializes a DECIMAL or DATE field to JSON and
JsonTreeCollectorreparses fractional numbers as double and dates as strings. An exact decimal such as 9007199254740993.01 can lose precision and type. This is a residual instance of the original typed-fallback concern. - 4141922700 (all legal legacy roots): FE permits an ARRAY to be cast to V1 VARIANT; native recursion rejects its VARBINARY leaf although a readable ASCII leaf works in UTF8 mode. The new TIMEV2 branch also emits legal negative or 24+-hour Doris duration values as Parquet Variant TIME_NTZ_MICROS. That primitive maps to Parquet TIME(false,MICROS), whose value means microseconds after midnight (Variant encoding, logical TIME); Doris's own Parquet TIME reader rejects those out-of-day values. These remain under the original legal-roots P1.
Critical checkpoints:
- Goal and proof: The feature adds opt-in Parquet Variant Flight results while retaining UTF8 by default. The added BE, FE, regression, and Python client tests cover principal scalar, composite, nested, slice, empty-result, schema, replay, and partition paths, but they do not cover the residual typed-document, ARRAY, out-of-day TIMEV2, or rollback cases. This review did not run tests or builds, so test execution claims in replies were not treated as independent evidence.
- Scope and parallel paths: The changes span the required FE session/capability/sink decision, optional Thrift fields, BE encoding/schema/IPC, GetTables, and client-facing tests. Both single and parallel result-sink creation, local and remote Flight readers, and ordinary UTF8 Arrow export were traced. No unrelated source change was identified.
- Concurrency and lifecycle: Heartbeats update a volatile capability and the sink pins a planning-time decision. The stale-capability rollback window above remains; no new lock-order defect, ownership cycle, or cross-translation-unit static initializer dependency was substantiated. Remote readers register the extension before IPC schema decode.
- Configuration and compatibility: The session flag defaults false, is forwarded, and affects subsequently planned Flight queries. Optional sink/heartbeat Thrift fields default false for old peers. Stable mixed-version membership falls back to UTF8; the transient routing case remains under P1 4141016458.
- Conditions and errors: Recursive extension binding, outer SQL-null masks, sliced root/document runs, and the documented 128-level native depth limit were checked. New Status results are propagated and the Arrow converter catches exceptions. The three residual value/compatibility cases above are the substantive failures.
- Testing and results: No
.outresult file changed. The added assertions do not prove the omitted negative cases; no runtime result was generated by this static review. No additional observability failure was substantiated; errors return Status and depth errors point to the UTF8 setting. - Persistence, data writes, and FE-BE passing: Heartbeat capability serialization/replay and downgrade handling were traced, and FE passes the pinned native choice in TResultSink. The PR does not change durable writes, transaction commit, or visible-version semantics.
- Performance and remaining concerns: Mixed V1 batches build root/document encodings and reparse document JSON; the demonstrated typed-value loss is already covered by P1 4141016465. No separate quantified performance blocker or unresolved candidate remained after the final sweep.
The user focus file specified no additional focus. This is a complete review execution based on static evidence, with no new inline comments because the substantiated remaining issues are covered by the three existing P1 threads.
Existing P0/P1 findings confirmed for this head: #68667 (comment), #68667 (comment), #68667 (comment)
|
run buildall |
|
/review |
|
Fixed the new P0 failure in Validation: the original ticket-decoding block failed focused tests for short ID halves. The updated block passes short/full-width IDs and canonical deduplication. Against a real local BE, a short ID reproduced HTTP 500, while normalized IDs returned HTTP 200 with the expected missing-query response. Groovy compilation passed, and the temporary BE was stopped. Full cancellation-suite validation remains for CI. |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Complete static review of PR #68667 at head ae5067f6a8cac85782d96019b8ffe8fdf63a5af4 (base 7b50e05cdbe352207b7f99084d3e0085eee3e168). I found two new P2 native Variant encoding defects, documented inline. Existing P1 comment #4141922700 still applies to a legal ARRAY<VARBINARY> legacy root: FE admits ARRAY to VARIANT, while the new recursive native encoder rejects its VARBINARY leaf after the UTF8 path can serialize an ASCII-byte example. That is the previously reported all-legal-roots concern, so I have not duplicated it inline.
Critical checkpoint conclusions:
- Goal and proof: The opt-in native Parquet Variant Flight path is connected from the FE session option and BE capability to Arrow schema, result sink, remote read, and ADBC transport. The added BE, FE, regression, and sample checks cover common legacy/V2, nested, null, and parallel cases. They do not cover the two new counterexamples or the remaining ARRAY case. I inspected tests and expected assertions; I did not run them, so runtime success is unverified here.
- Scope: Changes are focused on the native output path and its required FE/BE/Thrift compatibility, with related tests and sample documentation. No unrelated source change was identified.
- Concurrency and lifecycle: The heartbeat worker updates the volatile capability state; each query snapshots the session/capability choice in its result sink, and FlightInfo checks each endpoint's actual schema. The encoder adds no shared mutable state or locks. Its extension registration is function-local static, with no cross-translation-unit initialization dependency; buffers and writers are scoped to the result batch/query. A failed heartbeat can leave stale capability during downgrade, as already covered by an earlier rolling-upgrade thread.
- Configuration and compatibility: The session switch is evaluated per query. The optional Thrift capability/result-sink fields and heartbeat replay support mixed-version gating, and UTF8 remains the fallback. The existing P1 above shows that some valid legacy values still fail in native mode. Both parallel and nonparallel BE result-sender branches use the captured format bit; remote/proxy schema registration and FE GetTables nested metadata were checked.
- Conditions and error handling: Nullable and constant columns, scalar/document roots, nesting limits, sliced batches, schema binding, and recursive ARRAY/MAP/STRUCT paths were traced. Status and exception conversion exist at the new encoding boundary. The MAP-key collision and TIMEV2 domain checks identified inline are missing special cases; no further distinct defect was substantiated.
- Test results and observability: New assertions and regression expectations were reviewed statically, including the cancellation ticket and ADBC sample. No generated
.outchange was required by these tests. Conversion failures propagate through existing status/logging paths; I found no separate observability defect or untracked long-lived allocation. - Persistence, data writes, and performance: Heartbeat capability is journaled and replayed; this PR does not change transaction processing, storage visibility, MoW delete bitmaps, or data writes. The new recursion and row-batch allocation are query-scoped; no separately substantiated performance regression emerged.
- User focus: No additional review focus was supplied. All 26 authoritative changed paths and the unresolved-candidate list received a final sweep. Two full review rounds and separate risk scans converged with no further new valuable finding. This conclusion is static-only; no build, test, or source edit was performed.
Existing P0/P1 findings confirmed for this head: #68667 (comment)
|
run buildall |
|
/review |
There was a problem hiding this comment.
COMPLETE static review of PR #68667 at head 4f90a5a (base 7b50e05). One new P2 test-coverage finding is inline. No previously reported P0/P1 issue still applies to this head; the five existing P1 threads are addressed by capability gating, recursive typed legacy encoding, string-family handling, and the scalar null-root guard. Existing P2 MAP NULL-key and out-of-day TIMEV2 concerns now return explicit native-mode compatibility errors, and the documented depth limit has an actionable fallback. No other substantiated issue remains after two review rounds and the full changed-file sweep.
Critical checkpoints:
- Goal and proof: The opt-in session setting selects Arrow Parquet Variant for Flight result and nested Variant fields while the default remains UTF8. FE metadata and sink planning, BE schema creation, typed legacy/V2 writing, and remote schema restoration form the full path. Added BE and FE tests and Flight/ADBC examples cover many values and transport cases. This review was static only; no builds or tests were run, so their runtime results were not independently verified.
- Scope and clarity: The changes center on the Flight format and capability negotiation. The cancellation ticket ID adjustment is a related Flight test fix. The recursive legacy encoder is the main complexity; its root/document precedence was checked against the existing serializer.
- Concurrency: Heartbeat capability changes on the heartbeat path and is read through a volatile FE field. ResultSink captures the selected format during planning, and both local and parallel BE sender paths use that bit. No new shared BE lock, lock-order path, or heavy operation under a changed lock was found.
- Lifecycle: Extension registration uses a local static status, and the remote Flight reader registers it before IPC schema decoding. Result sender/schema lifetime and batch conversion were traced; no new ownership cycle or cross-translation-unit static initialization dependency was found.
- Configuration and compatibility: The session option defaults false, is forwarded, and affects newly planned queries. Optional sink and heartbeat thrift fields preserve older-peer defaults; FE requires every registered BE to advertise support before native mode. Native output has explicit documented limits for unrepresentable legacy values. In-flight tickets across BE downgrade remain a documented drain requirement.
- Parallel and conditional paths: Both sink setup paths pass the option; native and default UTF8, legacy and V2, nested containers, nulls, slices, and timezone aliases were checked. The new parallel regression can pass without multiple result BEs, which is the inline finding.
- Tests and results: BE tests cover nulls, exact decimals, nested roots/documents, MAP/TIMEV2 limits, depth, binary values and IPC; FE tests cover metadata, heartbeat replay/downgrade and mode capture. The new Groovy test checks native bytes on its Flight session, and the Python sample exercises ADBC direct and partition APIs. No .out file was changed. There is no independent runtime test result from this review.
- Observability and errors: New incompatibility paths return statuses with a UTF8 fallback setting; existing Flight status propagation carries conversion failures. No new critical long-running service path requiring a metric was identified.
- Persistence, writes and visibility: The BE capability is journaled/replayed through heartbeat state; the review found no new transaction, data-write, EditLog data, version-visibility, or delete-bitmap path. No new data corruption path was substantiated.
- FE/BE variables and performance: The new format bit is transmitted in TResultSink and consumed in both BE sink setup paths. Typed encoding avoids JSON reparse and loss of decimal/date identity. The per-batch builder work follows existing output batching; no substantiated hot-path regression or memory-ownership violation was found.
User focus: no additional focus was supplied. The accepted test gap is the only new inline point; all other candidates were dismissed with code or protocol evidence.
FE UT Coverage ReportIncrement line coverage |
4f90a5a to
46625fe
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Complete static review of PR #68667 at 46625fe (base 78aa9c9). I reviewed all 26 changed paths and the relevant FE/BE call chains, existing inline threads, fixtures, and three converged review rounds. The review found three new issues: [P1] native VARIANT disagrees with prepared/GetSchema analysis, [P2] a legacy JSONB leaf can bypass the combined 128-level depth limit, and [P2] an all-null nullable Decimal256 cast can fail before the SQL null mask is applied. All three are explained inline. There were no additional user focus points. This conclusion is static-only; I did not run builds or tests.
Critical checkpoint conclusions:
- Goal and proof: The opt-in path publishes native Parquet Variant for Flight while UTF8 remains the default. BE/FE, Groovy, and sample coverage exercise direct and partitioned output, but schema discovery/prepared execution and the two BE boundaries in the inline findings are not covered. The tests were inspected, not run.
- Scope and clarity: The edits are focused on Flight schemas, Variant serialization, capability negotiation, tests, and documentation. The cancellation fixture adjustment follows its ticket parser. No unrelated change requires a separate finding.
- Concurrency: Heartbeat updates and Flight execution use their existing synchronization boundaries. The new capability field is volatile and read during sink planning; no new lock order, blocking work under locks, or deadlock path was found.
- Lifecycle and initialization: Both sink construction paths capture the option for their query; local and remote readers register the extension and release through existing result lifecycles. No new cross-translation-unit static dependency or ownership cycle was found.
- Configuration: The session option defaults off and is evaluated for each newly planned Flight query; heartbeat capability can change as BEs report status. Its value is forwarded into the sink. An already planned ticket retains its captured schema, and the documented downgrade procedure requires draining active queries.
- Compatibility: Optional thrift fields and the all-registered-BE capability gate preserve UTF8 for older BEs. The new wire format is opt-in. Prepared and GetSchema analysis nevertheless continue to report UTF8 when execution uses the native format (P1).
- Parallel paths: Single and parallel result sinks, local and remote reads, nested Arrow fields, GetTables, and direct/partitioned reads were traced. GetTables and BE-produced schemas agree on supported types; prepared/GetSchema paths are the parallel entry points missed by P1.
- Conditions and error paths: Existing explicit limits for TIMEV2, MAP null keys, visible Decimal256, and deep legacy documents were checked. A JSONB leaf loses its enclosing depth (P2), and the scalar Decimal256 check precedes the outer SQL null mask (P2). Other masked and nested branches were rechecked without a separate substantiated issue.
- Tests and expected results: The 60-bucket fixture checks placement, unique/multiple endpoints when available, and per-stream schema equality; BE cases cover nested values, offsets, slices, nulls, and documented unsupported inputs. The missing cases are identified inline. No new .out result was added, and no runtime result is claimed from this review.
- Observability: Existing errors identify unsupported native values and the UTF8 fallback. The depth bypass can emit invalid output instead of its documented error (P2). No separate logging or metrics defect was substantiated.
- Persistence, transactions, writes, and versions: The heartbeat capability is included in FE response handling and replay. The feature does not change committed-data reads, transaction/data writes, partition visible versions, or MoW delete bitmaps.
- FE/BE variable propagation: The optional TResultSink bit reaches both BE sink construction branches, and the heartbeat bit reaches FE backend state. The schema-analysis omission is P1; no other missing transport point was found.
- Performance and remaining issues: Encoding cost is limited to opted-in Flight results; no concrete redundant hot-path work or memory ownership defect survived review. The final candidate list contains only the three inline findings.
Previously reported P1 comments 4141016458, 4141016465, 4141016481, 4141922700, and 4141922713 were checked against this head and no longer apply, so there are no existing blocking comment IDs. Related prior P2 threads on plain-document depth, nested timezone, TIMEV2, MAP null keys, and the parallel fixture were also deduplicated. Review complete; static evidence only.
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Review complete for eac03d1d196a49fe0a64a6f343e952f688c94415 after two convergence rounds and a final sweep of all 27 changed files. One new [P1] finding concerns heartbeat replay: a tolerated failure clears the new capability and journals a BAD response, causing follower FEs to mark a BE dead while the leader keeps it alive. The inline comment gives the concrete sequence and fix direction. No other new valuable finding was substantiated. No additional user review focus was specified.
Critical checkpoints:
- Goal and proof: The opt-in Flight native VARIANT schema, typed BE encoding, discovery/prepare paths, and remote extension handling are present. Added BE, FE, Groovy, and Python coverage exercises the intended shapes; the heartbeat replay case is missing. This review was static only: I ran no builds, tests, or cluster queries, and do not treat author-reported results as independent validation.
- Scope and clarity: The change is centered on Flight schema, sink transport, legacy/V2 conversion, and capability negotiation. The new legacy encoder is substantial but follows the existing root/document precedence; no smaller correctness issue was substantiated.
- Concurrency: BE output uses query-scoped builders. The new FE capability is volatile. Master heartbeat processing and follower journal replay are concurrent state paths; their liveness mismatch is the [P1] finding. No new lock-order or blocking-under-lock issue was substantiated.
- Lifecycle and static initialization: The sink pins the selected format for the query; remote readers register the extension before IPC schema decode. Registration uses function-local static state. No new ownership cycle or cross-translation-unit initialization dependency was found.
- Configuration:
enable_arrow_flight_sql_native_variantdefaults off and is evaluated for newly planned Flight queries; changes to the session affect later queries. The all-registered-BE heartbeat gate is conservative, but its failed-heartbeat persistence path has the reported defect. - Compatibility: Optional Thrift fields default to false, older BEs keep UTF8, and both result and proxy BEs are gated. Existing tickets across downgrade remain an operational limit documented by the PR. No new wire-format mismatch beyond the reported replay state issue was substantiated.
- Parallel paths and conditions: Serial and parallel result sinks receive the same flag; nested ARRAY/MAP/STRUCT fields and direct, prepared, GetSchema, and GetTables paths were traced. Legacy root visibility, SQL null masks, Decimal256, JSONB depth, MAP null keys, and TIMEV2 bounds have explicit handling or documented native-mode limits.
- Tests and expected results: New BE/FE tests and Groovy/Python integration assertions were inspected, including native/UTF8, nested, sliced, empty, and parallel cases. There is no changed
.outresult file. Add a leader/follower replay test for the inline issue; full cluster behavior remains unverified by this static review. - Observability: Unsupported-value errors point to UTF8 fallback, and heartbeat state changes are logged. No additional metric or logging blocker was substantiated.
- Persistence and writes: The PR adds heartbeat capability persistence but no table data or transaction write path. The EditLog replay/liveness error is reported inline; visible-version, delete-bitmap, crash-atomicity, and transaction checkpoints do not otherwise apply.
- FE/BE variables, errors, and performance: The option is carried through FE sink Thrift to both BE sink paths; status returns and the Arrow conversion exception boundary were checked. The scalar fast path and batched conversion avoid an obvious repeated JSON round trip; no separate performance or memory-safety defect was substantiated.
Existing inline threads were treated as duplicate fences. I independently checked the six earlier [P1] threads against this head: their capability gate, typed-root, string-root, legacy-null, and FE schema concerns are addressed, so no existing blocking comment ID is carried. Earlier [P2] threads likewise do not justify reposting. Review completion is static-only; the [P1] finding requires changes.
|
run buildall |
|
/review |
|
run buildall |
|
/review |
|
Fixed both issues exposed by the regression and local end-to-end validation:
Before pushing, I reproduced the failures locally, built the complete current-branch BE with the fix, and ran the full affected Groovy suite against a local 1 FE / 2 BE cluster in both V1 and V2 modes. Both passed. Python ADBC integration passed in both modes, and nested ADBC ExecuteSchema/Prepare/fetch checks passed with native mode false/true/false. All 51 selected BE tests and clang-format 16 also passed. Full cloud topology validation remains with CI. |
|
Codex automated review failed and did not complete. Error: Review recovery stopped: No final review submission was declared Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Complete static review of PR #68667 at e952ba3. The full 27-file diff was reviewed through two convergence rounds and a final changed-file sweep. Two new P2 findings remain in the legacy ARRAY root path that imports compute V2 Variant leaves; both are inline. Previously reported P1 issues were rechecked on this head and are addressed, so no existing blocking comment ID is carried forward. There was no additional user-provided focus.
Critical checkpoints:
- Goal and tests: The session opt-in, all-BE capability gate, FE schema discovery, BE native Arrow representation, and local/parallel/remote Flight paths are wired together. Added BE/FE unit tests, Groovy regression coverage, and the Python ADBC sample were inspected. This invocation ran no builds or tests; runtime and full-cluster behavior remain unverified by this review.
- Scope and conditions: The FE, BE, thrift, test, and documentation changes form the native Variant feature and its Flight regressions. The native format is pinned in the result sink; the conditional all-BE gate keeps unknown and older BEs on UTF8. SQL NULL, legacy null roots, masked unsupported values, nested types, and the documented native depth limit were checked. The nested V2 import still has the error gap reported inline.
- Concurrency and lifecycle: Flight connection schema analysis and execution synchronize on the connection context, while the heartbeat capability is volatile and cleared on journaled death/replay. Sink format is captured during planning, remote readers register the extension before IPC schema decoding, and result buffers retain their established lifecycle. No new lock-order, ownership-cycle, or cross-translation-unit static-initialization issue was substantiated.
- Compatibility and parallel paths: The new thrift fields are optional with false defaults; heartbeat replay and rolling-upgrade fallback were traced. Single and parallel result sinks, proxy readers, nested Arrow binding, GetTables, GetSchema, and prepared/direct query schemas were checked. Existing P1 comments about capability, schema, root typing, and heartbeat replay no longer apply.
- Data, persistence, and writes: This PR changes query result encoding and a journaled heartbeat capability, not table writes, transaction commits, visible versions, or delete bitmaps. The leader/follower heartbeat transition was checked against the existing journal contract. No data-write atomicity or EditLog replay gap was substantiated.
- Errors, performance, and observability: Native conversion has explicit unsupported-value errors for documented limits in the checked branches, except the nested V2 depth case below. The nested V2 shared-dictionary path has the quadratic validation cost reported below. Existing status/log paths provide query failure context; no additional metric or logging defect was substantiated. No result fixture was executed, so test expectations were assessed statically only.
Review status: COMPLETE. Two new P2 inline findings; no still-applicable prior P0/P1 findings. The review is static only.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Complete static review of PR #68667 at 70746f4. No new actionable findings. The prior P1 inline concerns are addressed on this head, so there are no still-applicable blocking comment IDs to carry. No additional user focus was supplied.
Critical checkpoints: The opt-in Flight SQL native VARIANT goal is implemented through the session gate, frozen TResultSink flag, BE Arrow schema and value writers, and local/remote Flight readers. The cross-module scope is necessary and remains focused. FE schema discovery, GetTables, Prepare, direct execution, parallel result sinks, and nested ARRAY/MAP/STRUCT fields were traced against the same capability and representation decisions. Scoped SET_VAR state uses the connection monitor; heartbeat capability is volatile, journaled, and replayed without changing tolerated-failure liveness. The sink captures the selected format for deferred fetching, while the remote reader registers the extension before decoding IPC. No new lock-order, lifecycle, or static-initialization issue was substantiated.
Compatibility and data correctness: The setting defaults to UTF8, optional thrift fields preserve older peers, and native output requires every registered BE to advertise support. Legacy root/document precedence, SQL null masks, exact typed leaves, V2 selected-value import, dictionary compaction, depth limits, and unsupported-value fallback were checked. Existing P1/P2 threads cover earlier problems in these paths and were not reposted. No transaction, table-write, visible-version, or EditLog data path changes are introduced; Backend capability persistence and replay were inspected. No new configuration propagation, observability, or performance issue was substantiated.
Coverage: BE and FE unit tests, the Flight SQL Groovy suites, Python ADBC sample test, and documentation were inspected, including negative boundaries, nested schemas, parallel endpoints, and cancellation-ticket normalization. Their assertions appear aligned with the intended behavior. This was a static-only review: no build, test, or cluster execution was performed, so runtime validation is left to CI and integration runs.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
2 participants
What problem does this PR solve?
Flight SQL exposes VARIANT as UTF8, preventing ADBC clients from receiving native binary Variant values. Add the default-off session variable
enable_arrow_flight_sql_native_varianton branch-4.1. When enabled on the querying Flight connection and every registered BE advertises support, Variant fields use thearrow.parquet.variantextension with non-nullable binarymetadataandvaluechildren. SQL NULL is a null struct; V2 Variant null is an encoded non-null value. Clients without a Variant extension implementation can access the physical struct and field metadata.Capture the option in the result sink and align nested result schemas, GetTables, GetSchema and prepared statements. Serialize reads of the option with scoped SET_VAR analysis so concurrent metadata calls cannot observe another query's temporary session. Unknown or older registered BEs keep new queries in UTF8 mode, including result proxies. Capability follows the last successful heartbeat during tolerated misses and is cleared with the existing death event, preserving leader/follower replay semantics. Successful unsupported reports also clear it. Heartbeat discovery cannot make a downgrade atomic: stop new native queries and drain outstanding tickets before replacing a BE. Heartbeat capability field 12 remains independent of Paimon's field 11.
Reuse the V2 binary representation and existing physical-value builder. Compact Flight dictionaries per selected row, preserving physical scalar types and decimal scales while avoiding replication of unrelated rows' keys. This also bounds nested one-row writes without repeatedly validating the shared dictionary. Iceberg output keeps its existing behavior. Legacy roots and documents use recursive typed encoding for dense/sparse paths, snapshots, MAP, STRUCT, ARRAY, VARBINARY, TIMEV2 and nested VARIANT. This preserves exact decimals, binary bytes, non-finite numbers and DATE identity without JSON reparsing. Legacy null roots remain empty objects, and pending defaults are read without mutating the source. Equivalent fixed-offset TIMESTAMPTZ labels bind to the published schema.
Native encoding accepts up to 128 nested levels. NULL map keys, TIMEV2 durations outside one day and visible Decimal256 roots return errors with UTF8 guidance because their semantics cannot be faithfully represented. The outer SQL null mask is applied before value conversion. MAP keys become object field names. Usage, limitations and client decoding behavior are documented in the Python Flight sample README.
Release note
Support opt-in native VARIANT results over Arrow Flight SQL / ADBC using
SET enable_arrow_flight_sql_native_variant = true. The default UTF8 mode is unchanged.Validation
test_flight_native_variantsuite passed against the fixed BEs withenable_variant_v2=falseandtrue, including serial/parallel results, native-mode toggles, Prepare/GetSchema, empty results and metadata size bounds. Nested ARRAY/MAP/STRUCT SQL runs in V2 because the analyzer rejects legacy Variant ARRAY arguments.Check List (For Author)