Repository navigation
Conversation
…ixes to master ### What problem does this PR solve? Forward-port only the requested changes whose missing behavior is still present on master, in branch-4.1 merge order: apache#68614, then apache#68667, followed by apache#68859. | Source PR | Evidence on master before this change | Action | | --- | --- | --- | | apache#68614 | Arrow ARRAY/MAP/STRUCT child fields lose Doris logical type markers; JSON/VARIANT markers are absent. | Preserve recursive logical metadata and use protocol-specific schema converters. | | apache#68667 | Flight maps Variant V2 to UTF8 instead of native binary values. | Return `arrow.parquet.variant` with `struct<metadata: binary not null, value: binary not null>` storage; preserve SQL NULL, physical scalar types and row-local dictionaries. | | apache#68852 | The obsolete external-table queries are already removed. All 276 active query tags match the 276 expected output blocks in execution order. | Excluded; no expected-output changes. | | apache#68856 | The preload module is already removed. The Hudi plugin resolves Parquet readers and format structures consistently to 1.18.0. | Excluded; no dependency changes. | | apache#68859 | Block metadata omits VARBINARY length, and deserialization constructs `DataTypeVarbinary()` with an unspecified length. | Preserve declared lengths, including nested and nullable columns, using optional protobuf field 13. | Adapt the ports to master's existing architecture: retain native UUID and nanosecond/timestamp mappings, reuse the shared FE Arrow mapper, and wrap extension storage correctly for nested Variant arrays. Master already uses Variant V2, so the removed legacy Variant switch and SerDe are not restored. Branch-4.1-only Iceberg Variant writing and prepared-statement schema machinery are outside this port. The Flight integration tests and Python samples cover native Variant and recursive metadata. The Flink regression explicitly casts Variant to STRING for a connector that declares that column as STRING. ### Release note Preserve nested logical type metadata in Arrow Flight SQL results, return Variant V2 as native Arrow Variant binary, and retain declared VARBINARY lengths across block serialization. Clients requiring Variant JSON text should explicitly cast it to STRING. ### Validation - FE targeted unit tests: 64 passed, including nested metadata, native Variant, UUID preservation and shared Arrow mappings. - FE Checkstyle: passed. - BE targeted ASAN unit tests: 326 passed, covering Arrow, Variant, UUID and VARBINARY. - Changed-line clang-tidy, clang-format 16 and build hygiene checks: passed. - Groovy regression scripts compiled; Python samples passed syntax checks. - Checked modified Thrift/protobuf field IDs against master and branch-3.1, branch-4.0, branch-4.1 and branch-4.2. - Confirmed the ES baseline query/output alignment and Hudi Parquet dependency tree before excluding those ports. - Live cluster, ADBC and Flink integration tests were not run locally. ### Check List (For Author) - Test - [x] Unit tests - [x] Regression tests added/updated - Behavior changed: - [x] Yes, native Variant Flight output and preserved type metadata. - Does this need documentation? - [x] Yes, Python Flight sample documentation updated. Adapted from commits: - e233c4a (apache#68614) - b8c03bf (apache#68667) - b35abbd (apache#68859)
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Complete static review of PR #68883 at head 91810cb36b7678adbb8f5fc2979827ff26841196. I found two P1 compatibility regressions, one P2 Flight error-status defect, and one P3 regression-test cleanup issue. The two P1s should be fixed before merging. There were no existing inline blockers to carry forward. Three review rounds converged with no unresolved candidate. No additional user focus was provided.
Critical checkpoints:
- Goal and proof: Native Variant V2 Arrow Flight output, nested logical metadata, and Varbinary width transport are implemented and covered by new BE/FE and regression cases, but the Paimon writer and external scanner regressions prevent the change from meeting its compatibility goal. The unchanged value-first Variant SerDe tests would fail under the new dispatch. Builds and tests were prohibited for this review, so these are static conclusions, not runtime results.
- Scope and clarity: The converter refactor reaches Flight, table-format, Python, and scanner consumers. Most production callers retain their protocol-specific schema; the memory scratch sink is a concrete unintended expansion (P1).
- Concurrency: Result schema converters are initialized before sink local-state use; the reviewed changes add no shared mutable request state or new lock order. No separate race or deadlock was substantiated.
- Lifecycle and ownership: Local and remote Flight readers obtain their schema from the result buffer, and the remote reader registers the Variant extension before decoding IPC. No new ownership cycle, static-initialization dependency, or missing release path was substantiated.
- Configuration: No new runtime configuration item is introduced. The new optional sink capability bit is set by FE Flight result sinks and read by BE; external scanner traffic has no corresponding negotiation (P1).
- Compatibility and wire definitions: Proto field 13 and Thrift field 4 are optional, retain their meaning on release branches where present, and have safe old-wire defaults. Native Variant changes the external scanner's established UTF8 representation without opt-in (P1). Old-BE UTF8 Variant is intentionally rejected by new FE, but its intended UNIMPLEMENTED status becomes INTERNAL (P2). The documented metadata difference between old-BE DoGet and enriched FlightInfo yielded no separate demonstrated client failure.
- Parallel paths: Legacy and Nereids planners both propagate the Flight sink bit; parallel and per-instance result sinks, nested SerDes, Paimon/Iceberg/Parquet writers, Python UDFs, and the separate memory scratch scanner were traced. Paimon's value-first Variant struct is lost by the new generic dispatch (P1).
- Conditions and errors: The new unconditional STRUCT branch makes the existing value-first branch unreachable (P1). The FE schema-fetch catch chain changes the intentional Flight status (P2). Other inspected Status returns and conditional bindings had no distinct substantiated issue.
- Test coverage and results: New tests cover root and nested Variant, nulls, dictionary compaction, metadata policy, timestamps, and Varbinary width. They miss the scanner's existing Variant wire contract, and the new regression suite deletes its table after failures contrary to repository test standards (P3). No builds, tests, or result regeneration were run because the review contract forbids execution.
- Observability: Existing sink statuses, reader errors, and profiles cover the new data paths; no separate logging or metric gap was substantiated. P2 obscures the actionable client error category.
- Persistence and transactions: No EditLog, transaction, rowset, or persistent storage format is changed. The Paimon failure concerns Arrow output before downstream write completion; no additional atomicity or crash-recovery defect was substantiated.
- FE/BE state transfer: The Flight metadata bit and Varbinary length are transmitted at their intended Thrift/protobuf boundaries; no other missing send path was substantiated.
- Performance and remaining issues: Per-row Variant dictionary compaction adds work but bounds emitted metadata and showed no separate correctness or material performance regression in static review. Final changed-file, inline-thread, and candidate sweeps found no further actionable point.
| options.timezone = &ctz; | ||
| const size_t first = checked_row(start); | ||
| const size_t last = checked_row(end); | ||
| if (array_builder->type()->id() == arrow::Type::STRUCT) { |
Contributor
There was a problem hiding this comment.
[P1] Preserve the value-first Variant struct writer. This branch captures every struct builder, but write_parquet_variant_arrow rejects the existing struct<value: binary, metadata: binary> layout before the following write_binary_variant_arrow branch can run. Paimon's Variant writer uses that value-first schema and delegates here, so Paimon Variant writes now fail; the unchanged binary-struct SerDe tests would fail too. Dispatch by the validated child field order to keep both representations reachable.
| } | ||
| DorisArrowBlockConvertor converter(block, state->timezone(), _timezone_obj, | ||
| /*datetime_naive=*/true); | ||
| ArrowFlightArrowBlockConvertor converter(block, state->timezone(), _timezone_obj); |
Contributor
There was a problem hiding this comment.
[P1] Keep the external scanner's Variant wire type compatible. TableQueryPlanAction installs this memory scratch sink for open_scanner, and get_next sends its RecordBatch directly to scanner clients. Switching to the Flight converter changes a selected Variant V2 column from UTF8 to an extension-backed STRUCT without any scanner capability flag; the in-tree DorisReaderExample accepts VARCHAR but rejects STRUCT. Keep this sink on the ordinary converter unless the separate scanner protocol negotiates native Variant.
| if (type.isVariantType()) { | ||
| if (field.getMetadata() == null | ||
| || !"arrow.parquet.variant".equals(field.getMetadata().get("ARROW:extension:name"))) { | ||
| throw CallStatus.UNIMPLEMENTED.withDescription( |
Contributor
There was a problem hiding this comment.
[P2] Preserve the unsupported Variant status at the Flight boundary. An older BE can return a UTF8 schema for a planned Variant, causing this branch to throw UNIMPLEMENTED. fetchArrowFlightSchema wraps that exception, then executeQueryStatement passes it to queryFailure, which returns INTERNAL to the client. Pass the capability error through the fetch and producer boundaries so clients receive the actionable UNIMPLEMENTED status.
| try { | ||
| allocator.close() | ||
| } finally { | ||
| jdbc_sql("DROP TABLE IF EXISTS ${table}") |
Contributor
There was a problem hiding this comment.
[P3] Leave the table available for regression debugging. This finally drop removes flight_native_variant_input even when an assertion fails. The suite already drops it before creation, and the repository testing standard requires retaining test tables afterward. Keep the client and allocator cleanup, but remove this final table drop.
Contributor
TPC-H: Total hot run time: 29054 ms |
Contributor
TPC-DS: Total hot run time: 151413 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28700 ms |
Contributor
TPC-DS: Total hot run time: 151428 ms |
Contributor
ClickBench: Total hot run time: 25.42 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
3 tasks done
Adapt the missing Variant hooks from apache#68381 to master, isolate Paimon binary storage from Flight, and decode native Variant through ADBC. Align GetTables and ADBC regressions and correct the Iceberg test wire fixture.
Gabriel39
requested review from
924060929,
CalvinKirs and
yiguolei
as code owners
October 11, 2026 01:03
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result
Complete static review of PR #68883 at 163e10248fe5f3cd255dfb909840beedcabd9101 (base a01cc89378f027b875a3f7e93b94ff04314e857f). Request changes: one new P1 finding is inline, and the already-reported P1 scanner issue remains applicable. The review covered the complete authoritative diff, related call paths, existing threads, and two convergence rounds. No additional user focus was supplied.
Existing threads: P1 4237385156 still applies because the memory-scratch scanner uses the Flight Variant converter without scanner negotiation. P1 4237385154 is resolved on this head: Paimon now calls its value-first Variant writer, including nested dispatch. P2 4237385157 still describes the new-FE/old-BE Flight status wrapping, and P3 4237385161 still describes the regression suite's final table drop; both are duplicate fences and are not reposted. The new inline P1 concerns the distinct old-FE/new-BE result-sink path.
Critical checkpoints
- Goal and proof: Native Arrow Variant transport and Doris type metadata are wired through Flight, ADBC, readers, and writers. The added unit and regression cases exercise native, nested, nullable, sliced, malformed, and empty results statically. The old-FE/new-BE result-sink compatibility gap means the goal is incomplete for rolling upgrades. No test was executed in this review.
- Scope and clarity: The changed paths are centered on Arrow schema/Variant transport, protocol fields, associated type handling, and focused tests/samples. I found no separate scope or abstraction issue worth an inline comment.
- Concurrency: Result buffers and their schema policy are scoped to a query; local and remote readers consume the stored schema. I found no new shared mutable state, lock-order change, or independently substantiated race.
- Lifecycle and ownership: Arrow buffers, extension restoration, sliced arrays, nullable values, and temporary Variant builders were traced through creation, conversion, and release. Malformed input returns an error before insertion; no distinct leak or dangling ownership path was substantiated.
- Configuration: There is no new dynamic configuration item. The optional FE-to-BE Flight flag selects schema metadata policy; its absent case does not preserve the previous Variant physical type, as the inline P1 explains.
- Compatibility and wire definitions:
PColumnMeta.varbinary_lengthid 13 andTResultSink.enable_arrow_type_metadataid 4 are optional and absent or same-meaning on origin branches 3.1, 4.0, 4.1, and 4.2. An absent Varbinary length remains-1. The remaining rolling-upgrade defects are the new inline P1 and the existing scanner/status threads. - Parallel paths: I checked both FE result-sink construction paths, ordinary and parallel BE result buffers, local and remote DoGet, schema RPC, GetTables, ADBC FileScannerV2, and Paimon/Iceberg/Parquet conversions. The Paimon writer thread is fixed; the scanner thread remains applicable.
- Conditional checks and errors: Native Variant shape/payload validation and unsupported-type errors have concrete failure paths. The existing P2 thread covers the Flight boundary's loss of an actionable UNIMPLEMENTED status.
- Tests and results: BE/FE unit tests, Flight/ADBC/Flink regressions, golden output, and sample clients were inspected. They cover the main new data shapes but the legacy-schema BE test codifies the old-FE/new-BE physical change; it does not prove compatibility. The existing P3 thread covers test cleanup. This review was static only: no build, test, or CI result is claimed.
- Observability: Existing status and logging paths appear sufficient for the new conversion paths apart from the already-reported Flight status wrapping. I found no additional metric or logging defect.
- Transactions and persistence: No transaction, EditLog, visible-version, or persisted catalog-state path changes here.
- Data writes: Paimon and Iceberg Arrow writer dispatch and fixed-width handling were traced. No new transaction atomicity or crash-recovery defect was substantiated; Paimon's prior value-first issue is resolved.
- FE/BE variables: Both FE Flight result-sink constructors set the optional flag, and BE reads it. Old FE plans omit it; the inline P1 is the resulting capability-handling defect.
- Performance and other issues: Schema recursion is proportional to schema shape; row conversion and Variant compaction paths revealed no separate substantiated CPU, memory, or correctness issue after the final sweep.
Existing P0/P1 findings confirmed for this head: #68883 (comment)
| Status ArrowFlightSchemaConvertor::convert_to_arrow_type( | ||
| const DataTypePtr& type, std::shared_ptr<arrow::DataType>* result) const { | ||
| // Flight always uses native Variant, including recursively converted children. | ||
| if (type->get_primitive_type() == TYPE_VARIANT) { |
Contributor
There was a problem hiding this comment.
[P1] Preserve UTF8 Variant for an FE without the Flight capability. An old FE omits enable_arrow_type_metadata, so ResultSinkOperatorX selects LegacyArrowFlightSchemaConvertor, but it inherits this native Variant mapping. The new BE consequently advertises and sends arrow.parquet.variant/STRUCT through both Flight readers, while the old FE GetTables still describes Variant as UTF8 and pre-upgrade clients expect JSON text. During an old-FE/new-BE rolling upgrade, discovery and query schemas disagree and the result wire type changes. Gate native Variant output on an explicit FE capability and keep the UTF8 path for older plans. This is separate from the existing memory-scratch scanner thread.
Contributor
TPC-H: Total hot run time: 29045 ms |
Contributor
TPC-DS: Total hot run time: 151648 ms |
Contributor
ClickBench: Total hot run time: 25.49 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
CalvinKirs
approved these changes
Oct 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Forward-port the requested changes whose missing behavior is still present on master: complete the missing Variant writer hooks from #68381, then retain the ports of #68614, #68667 and #68859 in branch-4.1 merge order.
value,metadataversus Flightmetadata,valuestorage contracts, including nested columns.arrow.parquet.variantwithstruct<metadata: binary not null, value: binary not null>storage; preserve SQL NULL, physical scalar types and row-local dictionaries.DataTypeVarbinary()with an unspecified length.Adapt the ports to master's existing architecture: retain native UUID and nanosecond/timestamp mappings, reuse the shared FE Arrow mapper, and wrap extension storage correctly for nested Variant arrays. Master already uses Variant V2, so the removed legacy Variant switch and SerDe are not restored. This does not add the branch-4.1-only Iceberg table Variant feature or prepared-statement schema machinery. ADBC recognizes the native Variant extension, validates and decodes its bytes, and retains Variant sub-column access; the scanner permits that carrier only for ADBC among Arrow readers.
The Flight integration tests and Python samples cover native Variant and recursive metadata. GetTables and ADBC expectations follow the native schema. The Iceberg static-null partition unit-test fixture uses master's existing Thrift field ID 19; production Thrift definitions are unchanged. The Flink regression explicitly casts Variant to STRING for a connector that declares that column as STRING.
Release note
Preserve nested logical type metadata in Arrow Flight SQL results, return Variant V2 as native Arrow Variant binary, preserve native Variant values and sub-column access through ADBC, and retain declared VARBINARY lengths across block serialization. Clients requiring Variant JSON text should explicitly cast it to STRING.
Validation
test_adbc_type_semantic,test_get_tables_schemaandtest_flight_native_variantpassed (3 suites, 0 failures, 0 skips). The ADBC.outwas generated by the regression framework and then checked by a normal rerun.Check List (For Author)