branch-4.1: [feature](paimon) Support Variant V2 writes - #66321
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.
Request changes: three P1 defects and one P2 repository-contract issue.
Correctness and analysis:
- Heterogeneous multi-row VALUES are coerced to one common primitive type before the Variant sink cast, silently changing per-row Variant types.
- The Paimon analyzer accepts Variant V2 source shapes and scalar types that the BE kernel cannot execute, including legacy leaves hidden by shape changes and FE-admitted unsupported sources.
- The new BE Arrow test constructs an explicitly unsupported typed TIMEV2 Variant and deterministically aborts before serialization.
Transport and compatibility:
- I traced encoded, typed, const, nullable, sliced, and nested Variant values through the C++ Arrow builders, IPC ownership, Java conversion, and Paimon compatibility walk. Aside from the failing test input above, byte ownership, null/child alignment, and the private Doris-produced compatibility boundary did not reveal another defect.
- The table-wide enable_variant_v2 gate matches the PR's documented opt-in policy.
Repository guidance:
- All four touched checkLegalityBeforeTypeCoercion overrides still bypass the required getArgument/getArguments accessors.
Tests and validation:
- This was a static-only review as required by the runner instructions; no local builds or tests were run. The added coverage is broad, but it lacks regressions for the accepted analysis/coercion cases and contains the deterministic test-construction failure called out inline.
User focus and completion:
- No additional user-provided review focus was supplied, so the complete 29-file PR diff was reviewed.
- Review completed after three bounded convergence rounds, final changed-file and unresolved-candidate sweeps, and a live head/duplicate check.
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
6d244a3 to
fa39ea6
Compare
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Static review of the full 49-file change at c5bacf78662f73a05ac5ddf2df92eb92f3da6f85 found six blocking issues: the new BinaryBuilder transport can overflow on a batch of individually valid Variants; the Paimon 1.4.2 bump breaks mixed-version serialized table/split payloads; the Docker fixture still lacks the newly mounted 1.4.2 jars; the Java-UDF assembly leaves an orphan standard-JDK chronology provider descriptor; FE admits non-empty ARRAY although the BE encoder rejects it; and the dynamic-bucket regression cannot detect key/payload permutation.
I also traced the required legality-check accessors, nested Variant/null/ownership paths, Paimon wrapper precedence, overwrite binding/commit lifecycle, SDK artifacts, and the remaining regression surface. Those paths did not yield additional non-duplicate findings. There was no additional user-provided review focus, so the whole PR was reviewed. The repository code-review skill was absent from this checkout; the authoritative review bundle and required AGENTS policy were followed directly. This was a static-only review; no builds or tests were run.
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run external |
1fed9fc to
3cfafb2
Compare
|
run buildall |
553c6f7 to
9a32163
Compare
9a32163 to
ea5fa27
Compare
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
suxiaogang223
changed the title
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
1 similar comment
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Review result
Requesting changes for four distinct pre-sink Variant V2 type-loss mechanisms, grouped into two inline comments at their shared changed-line anchors:
- Container unification can scalarize a V2 leaf both inside a heterogeneous
Array/CreateMapand later when indexed-Any array functions combine independently homogeneous arrays. - Result coercion can scalarize V2 before Paimon validation through heterogeneous set/conditional/selector expressions, while one-argument
LEAD/LAGcan select a trivial scalar signature and turn object/array roots intoNULL.
I did not duplicate the current-head threads covering the non-Parquet Variant format fence, aggregate Arrow/IPC sizing, repeated shared-metadata validation/copying, mixed Paimon serialization versions, or the ServiceLoader descriptor. The final static sweep also confirmed the changed legality hooks use getArgument(s), recursive no-op layout checks retain exact container/field structure, writer and committer share the configured overwrite binding, fallback order matches the adjacent factory precedence, and the changed tests reach their intended existing fixes.
This review is capped and incomplete rather than converged: the third and final permitted round produced two new valuable findings, and the review contract permits no fourth round. All four substantiated mechanisms are reported here, but I cannot claim a clean convergence round for this head.
No additional user-provided focus was specified. No builds or tests were run, as required by the review contract; this was a full static review of the authoritative bundle at 027e11f67c1296d78118a87d5a31be3bd6b9935a.
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
the
approved
|
skip buildall |
github-actions
Bot
removed
the
approved
|
skip buildall |
### What problem does this PR solve? Issue Number: close apache#65086 Related PR: apache#66321 Problem Summary: Forward-port Variant V2 serialization for Paimon Arrow batches, including encoded value/type byte pairs, outer-null preservation, supported nested constructors, and array-to-Variant conversion behavior. ### Release note Support writing Variant V2 values to Paimon tables. ### Check List (For Author) - Test: Unit Test - Added serialization and cast coverage; BE format/check and FE checkstyle passed. Final compilation and test execution are deferred until all forward-port picks are complete. - Behavior changed: Yes, Variant V2 values can be serialized for Paimon writes. - Does this need documentation: No
### What problem does this PR solve? Issue Number: close apache#65086 Related PR: apache#66321 Problem Summary: The Variant V2 forward-port referenced a branch-specific arguments field while validating ARRAY children. Master exposes legality-check inputs through getArguments(), so FE compilation failed. Use the master expression API while preserving validation of every array argument. ### Release note None ### Check List (For Author) - Test: Manual test - FE compilation identified the issue; final incremental build follows this commit - Behavior changed: No - Does this need documentation: No
4 participants
What problem does this PR solve?
Issue Number: Part of #65086
Problem Summary:
The Paimon JNI writer already supports primitive and complex Doris columns, but a Doris Variant V2 value could not be written losslessly because the generic Arrow path exposed Variant as JSON text. That loses the native Variant
valueandmetadatarepresentation and does not support Variant nested in ARRAY, MAP, or STRUCT.This PR adds a Paimon-specific Variant V2 write protocol:
enable_variant_v2=trueand rejects legacy Variant V1 inputs during analysis with an actionable error;struct<value: binary, metadata: binary>while preserving SQL NULL separately from a Variant null value;GenericVariantin the Java JNI backend and uses Paimon SDK accessors to validate compatibility with the bundled Paimon version;Supported write scenarios
INSERT INTO ... VALUESandINSERT INTO ... SELECT, including UNION inputs and reordered/omitted columns.INSERT OVERWRITEand static partition writes.typed_valuelayout and values, Paimon unshredding, type-mismatch fallback to residual bytes, and reading a table containing both unshredded and shredded files.Current limitations
enable_variant_v2=false, or when a legacy Variant V1 expression is supplied to a Paimon VARIANT target, analysis fails intentionally.Release note
Support writing Variant V2 values, including nested Variant values, from Doris into Apache Paimon tables through the Java JNI writer. Set
enable_variant_v2=truebefore writing a Paimon table containing VARIANT.Check List (For Author)
Test
./run-regression-test.sh --run -s test_paimon_write_variant_shredding./run-regression-test.sh --run -s test_paimon_write_variant_table_modesBehavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)