[refactor](arrow) Make format convertors explicit and schema-owned - #68381
Conversation
### What problem does this PR solve? Related PR: apache#67784. This is the second split, based on the primitives merged in apache#68297. Arrow batch conversion mixes protocol serialization with table-specific UUID and Variant handling, while writers construct schemas separately. Introduce explicit Doris, Python, Arrow Flight, Parquet, Hive, Iceberg and Paimon convertors, instantiated with their own schema parameters and timezone. Move table schema construction/decoding into the convertors and route nested SerDe writes through the selected format. Separate Parquet, Hive and Iceberg writers and migrate all callers. Preserve current Parquet timestamp encoding, UUID/Variant layouts and external type mappings. Keep Python's numeric conversion timezone consistent with its declared Arrow schema, and reject incompatible nested target schemas before casts or child access. No FE, Thrift, ORC timestamp, or binary computation changes are included. ### 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) - Test - [x] Unit Test: schema ownership/isolation, slices, invalid schemas, nested/null values, UUID/Variant bytes, fixed-offset Python batches, and Parquet timestamp representation. - [x] Regression test: add Python UDF/UDTF/UDAF timezone assertions and extract the corresponding existing Python snapshot corrections from apache#67784. - Behavior changed: - [x] Yes: align Python UDF conversion with its Arrow timezone declaration; report invalid nested schema bindings as errors. - Does this need documentation? - [x] No. Validation: ASAN BE build and 396 selected tests passed; the final incremental build and 285 focused tests also passed. clang-format 16 and Groovy/embedded Python syntax checks passed. Python SQL and external-catalog end-to-end regressions were not run locally and remain for CI. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex exited with status 0 without a terminal turn event; review is incomplete Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
### What problem does this PR solve? Related PR: apache#68381 The timezone regression declares ARRAY<STRUCT<value:STRING>> but compares its lateral-view output directly to a string. Declare ARRAY<STRING> to match the single string yielded per row and allow the comparison to reach execution. Preserve coverage for fixed-offset timezones, microseconds, pre-epoch values and NULLs. ### Release note None ### Check List (For Author) - Test: Groovy syntax and embedded Python scalar, UDTF and UDAF value checks; SQL analysis verification. - Behavior changed: No (test-only correction). - Does this need documentation: No.
|
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 exact head c552895e6c9dbdece9ff4bfab484a7086cd4555e.
I found two blocking correctness issues:
- Protocol-specific Paimon/Iceberg complex writers validate only the outer container, so an incompatible nested target can reach the wrong builder or silently reinterpret decimal scale/field identity.
- The new plain timestamp binding check rejects the supported zero-offset session alias chain (
+00:00->Z->Zulu/UTC) even though the zones are semantically identical.
Coverage included all 51 changed paths plus the relevant scalar SerDes, FE-to-BE timezone propagation, Paimon JNI schema ownership, Iceberg/Parquet writer lifetimes and cleanup, Arrow Flight, Python callers, build discovery, and changed tests/regressions. The writer split, schema lifetimes, cleanup/statistics paths, UUID/VARBINARY handling, and nonzero/DST timezone cases did not yield another distinct issue. No user-specific focus item was provided, and no live review/comment duplicate existed immediately before submission.
Validation was static only, as required by the review contract; no builds or tests were run.
| arrow::ArrayBuilder* array_builder, int64_t start, int64_t end, | ||
| const cctz::time_zone& ctz) const { | ||
| // Reject an incompatible target before casting its nested schema or builder. | ||
| if (field->type()->id() != arrow::Type::LIST || |
There was a problem hiding this comment.
[P1] Validate the nested binding before dispatching to the child SerDe
This only checks that the outer target is a LIST. The Paimon/Iceberg converters bypass the recursive plain-binding validator, so a same-outer-kind mismatch reaches the child writer. For example, a Doris array<decimal(10,3)> value 1.234 has raw coefficient 1234; if the pinned target is list<decimal(10,2)>, the Decimal128 writer accepts the same builder and persists that coefficient as 12.34. array<int> versus list<string> instead reaches the wrong builder cast, and Map/Struct have the same gap. Please validate the full recursive binding (including decimal parameters, timestamp units, and struct field identity/order) before appending the parent builder.
| // with DATETIMEV2; TIMESTAMPTZ must always retain its instant semantics. | ||
| if (target_timestamp.timezone().empty()) { | ||
| return primitive == TYPE_DATETIMEV2; | ||
| } |
There was a problem hiding this comment.
[P1] Compare timezone bindings semantically instead of by cctz name
A valid +00:00 session reaches BE as Java's canonical Z. The timezone cache resolves Z to a cctz object named Zulu, while Arrow schema construction rewrites the declared Z label to UTC. write_plain_arrow_column therefore builds the source timestamp with Zulu, this line resolves the target as UTC, and the new preflight rejects UTC == Zulu before writing even though both are the same zero-offset zone. This breaks timezone-aware timestamp batches on the common plain converter path (including Parquet/Hive and memory-scratch/Arrow-Flight consumers) for the supported +00:00 session value. Please normalize both sides to one identity or compare actual zone equivalence, and add a full batch-conversion test using the cctz object resolved from Z rather than only checking the generated schema label.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…68396) ### 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<STRING>, 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) - Test - [x] Unit Test: explicit schemas and independent converter instances, nested/null values, UUID and fixed binary bytes, timestamp bindings, and Iceberg writer statistics. - [x] Regression test: Python UDF/UDTF/UDAF timezone comparisons and the existing timestamp snapshot corrections. - Behavior changed: - [x] Yes: align Python UDF conversion with its Arrow timezone declaration; reject invalid nested bindings before casts. - Does this need documentation? - [x] No. 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) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
|
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. |
…68381) ### What problem does this PR solve? Related PR: #67784. This is the second split, based on the primitives merged in #68297. Arrow batch conversion mixes protocol serialization with table-specific UUID and Variant handling, while writers construct schemas separately. Introduce explicit Doris, Python, Arrow Flight, Parquet, Hive, Iceberg and Paimon convertors, instantiated with their own schema parameters and timezone. Move table schema construction/decoding into the convertors and route nested SerDe writes through the selected format. Separate Parquet, Hive and Iceberg writers and migrate all callers. Preserve current Parquet timestamp encoding, UUID/Variant layouts and external type mappings. Keep Python's numeric conversion timezone consistent with its declared Arrow schema, and reject incompatible nested target schemas before casts or child access. No FE, Thrift, ORC timestamp, or binary computation changes are included. ### 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) - Test - [x] Unit Test: schema ownership/isolation, slices, invalid schemas, nested/null values, UUID/Variant bytes, fixed-offset Python batches, and Parquet timestamp representation. - [x] Regression test: add Python UDF/UDTF/UDAF timezone assertions and extract the corresponding existing Python snapshot corrections from #67784. - Behavior changed: - [x] Yes: align Python UDF conversion with its Arrow timezone declaration; report invalid nested schema bindings as errors. - Does this need documentation? - [x] No. Validation: ASAN BE build and 396 selected tests passed; the final incremental build and 285 focused tests also passed. clang-format 16 and Groovy/embedded Python syntax checks passed. Python SQL and external-catalog end-to-end regressions were not run locally and remain for CI. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
3 participants
What problem does this PR solve?
Related PR: #67784. This is the second split, based on the primitives merged in #68297.
Arrow batch conversion mixes protocol serialization with table-specific UUID and Variant handling, while writers construct schemas separately. Introduce explicit Doris, Python, Arrow Flight, Parquet, Hive, Iceberg and Paimon convertors, instantiated with their own schema parameters and timezone. Move table schema construction/decoding into the convertors and route nested SerDe writes through the selected format.
Separate Parquet, Hive and Iceberg writers and migrate all callers. Preserve current Parquet timestamp encoding, UUID/Variant layouts and external type mappings. Keep Python's numeric conversion timezone consistent with its declared Arrow schema, and reject incompatible nested target schemas before casts or child access. No FE, Thrift, ORC timestamp, or binary computation changes are included.
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 396 selected tests passed; the final incremental build and 285 focused tests also passed. clang-format 16 and Groovy/embedded Python syntax checks passed. Python SQL and external-catalog end-to-end regressions were not run locally and remain for CI.
Check List (For Reviewer who merge this PR)