[refactor](arrow) Make format convertors explicit and schema-owned - #68396
Merged
yiguolei merged 1 commit intoSep 23, 2026
Merged
Conversation
### What problem does this PR solve? Related PR: apache#68381, apache#67784 Separate Arrow conversion by protocol and move schema construction into per-instance convertors. Migrate Parquet, Hive, Iceberg and Python/Flight callers, retain master memory tracking and timestamp-nanosecond support, and correct the Python timezone UDTF declaration to match its string output. Adapt the branch-4.1 refactor to the existing master surfaces without importing the Paimon write backend or physical Variant table writes. Preserve current Parquet timestamp representation and Iceberg statistics. ### Release note Fix Python UDF timestamp conversion to preserve wall-clock values with a fixed-offset Arrow timezone. ### Check List (For Author) - Test: ASAN BE build; 310 focused BE tests passed; clang-format 16 and header hygiene passed; corrected UDTF SQL analysis and Groovy/embedded Python checks passed. Full SQL regressions remain for CI. clang-tidy is blocked by an existing unmatched NOLINTEND in core/types.h. - Behavior changed: Yes, align Python timestamp values and protocol metadata and reject incompatible nested bindings. - Does this need documentation: No. Adapted from commits c070334 and c552895.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review found two requested changes.
Findings
- P2: the new Paimon target protocol drops the target timestamp unit in its scalar fallback. Precision-7-through-9 Paimon timestamps use an Arrow nanosecond field while Doris caps the source type at scale 6, so this interface will append microsecond counts to a nanosecond builder when the write backend is wired.
- P2: the new deterministic Python timezone regression uses direct Groovy assertions and has no generated result snapshot, contrary to the repository regression-test rules.
Checkpoint conclusions
- Goal, scope, and focus: the schema-owned converter split and generic/Hive/Iceberg writer migration are cohesive across all 46 changed paths. No user-specific focus was supplied, so the complete PR was reviewed. The Paimon target contract is not complete for nanosecond timestamps.
- Concurrency and lifecycle: converter instances are call- or writer-local; no new shared mutable state, lock ordering, bthread boundary, or static-initialization dependency was found. Runtime timezone state and schema JSON are copied where required, Iceberg schema owners outlive their writers, and writer-close/stream-close/statistics ordering is preserved.
- Error handling and memory safety: column counts and slice endpoints are checked, target shape/UUID/fixed-width failures propagate through Status, local builders are discarded on failure, and Arrow allocations continue to use the supplied pools. No new ownership leak, dangling reference, or swallowed error was found.
- Data correctness and nullable handling: ordinary/Python/Flight/Parquet/Hive/Iceberg timestamp, UUID, nested Array/Map/Struct, null-map, const, and nonzero-slice paths were traced. The Paimon nanosecond mismatch above is the remaining correctness defect; no distinct Python timezone, nullability, or Iceberg mapping defect survived verification.
- Compatibility, configuration, and persistence: Parquet encodings, Iceberg field IDs/schema metadata/statistics, file cleanup, and renamed callers are preserved. There is no new configuration, FE-BE variable, EditLog/replay, transaction, rolling-upgrade, cloud-mode, or merge-on-write surface.
- Observability and performance: synchronous failures retain contextual Status/Arrow errors; no new distributed transition needs a metric. Per-batch schema/builder work replaces equivalent prior work, with no substantiated hot-loop or unbounded-allocation regression.
- Tests: unit coverage is broad but omits the Paimon precision-9 target case. The new SQL regression must use named qt_/order_qt_ cases and a runner-generated .out file. This review was static only as required: no build or test was run here; author/CI validation claims were not independently reproduced.
| const std::shared_ptr<arrow::Field>& field, | ||
| arrow::ArrayBuilder* array_builder, int64_t start, | ||
| int64_t end, const cctz::time_zone& ctz) const { | ||
| return serde.write_column_to_paimon_arrow(type, column, null_map, |
Contributor
There was a problem hiding this comment.
[P2] Bind timestamps to the Paimon target unit. This new protocol is not wired to master's Paimon backend yet, but its advertised target-schema contract is already incorrect: Paimon 1.3 maps TIMESTAMP precision 7-9 to an Arrow nanosecond field, while Doris maps those columns to scale 6. The default hook derives a microsecond count only from the Doris scale and ignores the builder's NANO unit, so the int64 append succeeds but a value such as 2023-04-20 00:00:00.123456 is 1,000x too small; nested/nullable timestamps take the same path. Please scale from the target TimestampType::unit() (including negative epochs), or reject the binding, and add a precision-9 target test before this interface is consumed.
| // Check values observed inside Python as well as returned values to detect offset cancellation. | ||
| ["UTC", "+05:45", "-03:30", "Asia/Shanghai"].each { zone -> | ||
| sql "SET time_zone = '${zone}'" | ||
| assertEquals([[true], [true], [true]], sql(""" |
Contributor
There was a problem hiding this comment.
[P2] Use a generated query-test result for these fixed expectations. This new regression compares deterministic rows with assertEquals, but the repository's regression-test rules require determined results to use qt_/order_qt_ and commit the runner-generated .out snapshot. Please express the zone/function cases as named query tests and generate the corresponding result file.
yiguolei
approved these changes
Sep 23, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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?
Related PR: #68381. This is the master version of the second split from #67784, based on the primitives merged in #68301.
Arrow batch conversion mixes protocol serialization with table-specific UUID handling, while writers construct schemas separately. Introduce explicit Doris, Python, Arrow Flight, Parquet, Hive, Iceberg and Paimon convertors with instance-owned schema parameters and timezone. Move schema construction/decoding into the convertors and route nested SerDe writes through the selected format.
Separate Parquet, Hive and Iceberg writers and migrate existing callers. Preserve master's tracked Arrow memory pools, Iceberg statistics and timestamp-nanosecond support. Master does not yet contain the Paimon write backend or physical Variant table writes present on branch-4.1; this pick adds the converter interfaces without importing those features. Parquet timestamp encoding and external type mappings remain unchanged.
Include the Python timezone regression correction from #68381: the single string output uses ARRAY, so the lateral-view comparison reaches execution instead of failing on a STRUCT-versus-STRING comparison. Retain coverage for four session timezones, microseconds, pre-epoch values, NULLs, UDF, UDTF and UDAF.
Release note
Fix Python UDF timestamp conversion to preserve wall-clock values when the Arrow protocol declares a fixed-offset timezone.
Check List (For Author)
Validation: ASAN BE build and 310 focused tests passed (53 suites), covering Arrow conversion, Parquet/ORC, Variant SerDe and Python. All 42 affected C++ files passed clang-format 16, and header hygiene passed. The original UDTF declaration reproduced the SQL analysis error on an isolated FE; the corrected declaration passed the same analysis. Groovy and embedded Python checks passed. Full Python SQL and external-catalog regressions remain for CI. clang-tidy was attempted: the new converter's size warning was resolved; analysis remains blocked by a pre-existing unmatched NOLINTEND in core/types.h.
Check List (For Reviewer who merge this PR)