[Coral-Schema] Reconstruct multi-branch unions from Iceberg union-structs - #610
Open
yyy1000 wants to merge 4 commits into
Open
[Coral-Schema] Reconstruct multi-branch unions from Iceberg union-structs#610yyy1000 wants to merge 4 commits into
yyy1000 wants to merge 4 commits into
Conversation
…ucts
Iceberg has no union type, so a Hive uniontype<A,B,C> persisted into an
Iceberg table surfaces as the struct {tag:INT, field0:A, field1:B, field2:C}
(the Trino union representation, trinodb/trino#3483 -- the same encoding
HiveToCoralTypeConverter.convertUnion produces), while the partner Avro still
describes the column as a real union [null,A,B,C]. MergeCoralSchemaWithAvro
previously could not match the union partner, so it synthesized a record:
multi-branch unions collapsed and union-typed field defaults failed outright.
Add a union-struct reconstruction path:
- unionPartnerOrNull detects the case when all of (1) the Coral struct is
union-shaped (tag:INT then field0..fieldN-1), (2) the partner is an Avro
union, and (3) the partner's non-null branch count equals the member count.
A memberCount >= 2 guard makes detection collision-free: a genuine nullable
struct yields [null, record] (one non-null branch), which can never equal a
member count of two or more. Single-member union-structs fall back to the
struct path.
- mergeUnionStruct merges each fieldN member against the partner union branch
by ordinal, strips per-member null wrappers (the union's own NULL branch
carries nullability), and emits the NULL branch first when the partner union
has one -- matching MergeHiveSchemaWithAvro.union so the Iceberg path stays
faithful to the legacy Hive Avro baseline. Emitting a real union also lets
the partner's field default apply exactly as on the Hive path.
Adds six tests covering multi-branch reconstruction, the no-null-branch case,
arrays of unions, the union-default regression, and a negative case proving a
genuine record partner is not mistaken for a union.
simbadzina
reviewed
Jun 26, 2026
…ion member tests Two review threads from @simbadzina. 1. unionPartnerOrNull only ever returned its own `partner` argument or null, making the @nullable Schema return a boolean in disguise and forcing the caller to re-derive the meaning from a null check. Rename to isMultiBranchUnionStruct, return boolean, and let the caller pass `partner` directly; this drops the @nullable return and the local alias. Updated the two {@link} references in isUnionStruct and mergeUnionStruct. 2. Both existing union tests used primitive members only, so neither the per-branch merge against the partner nor the extractIfOption unwrap was covered for non-primitives. Add three cases: - record member: fields merged from the partner branch, and the branch is not double-wrapped in its own [null, record] option - array member: stays an array branch rather than being collapsed - string member with an enum partner branch: promotes to the partner ENUM, confirming branches use the normal promotion path Verified these have teeth by mutation testing. Removing the extractIfOption unwrap fails all three; removing the per-branch partner merge fails the record and enum cases while every pre-existing test still passes -- which is exactly the blind spot the review identified. coral-schema suite green: 152 tests, 0 failures.
yyy1000
pushed a commit
to yyy1000/coral
that referenced
this pull request
Aug 6, 2026
…ion member tests Two review threads from @simbadzina. 1. unionPartnerOrNull only ever returned its own `partner` argument or null, making the @nullable Schema return a boolean in disguise and forcing the caller to re-derive the meaning from a null check. Rename to isMultiBranchUnionStruct, return boolean, and let the caller pass `partner` directly; this drops the @nullable return and the local alias. Updated the two {@link} references in isUnionStruct and mergeUnionStruct. 2. Both existing union tests used primitive members only, so neither the per-branch merge against the partner nor the extractIfOption unwrap was covered for non-primitives. Add three cases: - record member: fields merged from the partner branch, and the branch is not double-wrapped in its own [null, record] option - array member: stays an array branch rather than being collapsed - string member with an enum partner branch: promotes to the partner ENUM, confirming branches use the normal promotion path Verified these have teeth by mutation testing. Removing the extractIfOption unwrap fails all three; removing the per-branch partner merge fails the record and enum cases while every pre-existing test still passes -- which is exactly the blind spot the review identified. coral-schema suite green: 152 tests, 0 failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
yyy1000
force-pushed
the
iceberg-first-avro/pr4-multibranch-union
branch
from
August 6, 2026 18:28
52a68de to
d07c8ec
Compare
simbadzina
previously approved these changes
Aug 10, 2026
Collaborator
There was a problem hiding this comment.
LGTM.
Beyond the tests in the PR, Claude did some additional corner-case checking locally:
- Probed ~25 edge cases around the union-struct path: unions nested in arrays/maps and inside regular structs, non-contiguous
fieldN, case variations ontag/fieldN, member/branch count mismatches, null placement in the partner union, and partner branch order divergence. The false-positive guards hold up — a genuine nullable struct that happens to reuse thetag/field0names still comes out a record. - Ran each of those against the legacy
MergeHiveSchemaWithAvropath for comparison. No regressions: everywhere the new code throws, legacy throws the same way, and on arity drift the new code actually degrades more gracefully than legacy (which throwsIndexOutOfBoundsException). - Ran the full multi-module build locally against d07c8ec — all modules green, coral-schema 152 tests passing.
simbadzina
self-requested a review
August 10, 2026 17:58
aastha25
reviewed
Aug 12, 2026
Comment on lines
91
to
+96
| case STRUCT: | ||
| return mergeStruct((StructType) coralType, partner); | ||
| StructType structType = (StructType) coralType; | ||
| if (isMultiBranchUnionStruct(structType, partner)) { | ||
| return mergeUnionStruct(structType, partner); | ||
| } | ||
| return mergeStruct(structType, partner); |
Contributor
There was a problem hiding this comment.
where are we taking care of nullable single uniontype ? there are 4 cases in total -
(1) nullable complex union -
(2) non-nullable complex union
(3) nullable single union
(4) non-nullable single union
aastha25
reviewed
Aug 12, 2026
Comment on lines
+161
to
+174
| private boolean isUnionStruct(StructType structType) { | ||
| List<StructField> fields = structType.getFields(); | ||
| if (fields.size() < 2) { | ||
| return false; | ||
| } | ||
| StructField tag = fields.get(0); | ||
| if (!"tag".equals(tag.getName()) || tag.getType().getKind() != CoralTypeKind.INT) { | ||
| return false; | ||
| } | ||
| for (int i = 1; i < fields.size(); i++) { | ||
| if (!("field" + (i - 1)).equals(fields.get(i).getName())) { | ||
| return false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
is the login assuming that we dont see a nullable compelx union - {null, tag, field0, field1...} or is it expected that structType will not be nullable? is the latter, we should document it in the method java docs.
aastha25
reviewed
Aug 12, 2026
Comment on lines
+167
to
+171
| if (!"tag".equals(tag.getName()) || tag.getType().getKind() != CoralTypeKind.INT) { | ||
| return false; | ||
| } | ||
| for (int i = 1; i < fields.size(); i++) { | ||
| if (!("field" + (i - 1)).equals(fields.get(i).getName())) { |
Contributor
There was a problem hiding this comment.
lets do case insensitive checks
…on, document null handling Two review comments from @aastha25. isUnionStruct matched the "tag" and "fieldN" marker names case-sensitively, so a union-struct arriving through a catalog that normalizes field casing would fail detection and be silently downgraded to a record. Match case-insensitively. mergeUnionStruct ignores unionStruct.isNullable() and takes nullability solely from the partner union. That was intentional but undocumented: for a union the partner Avro is the authority on the envelope, and a nullable Hive uniontype rolls its null into the members, surfacing as {tag, field0, ...} with optional members rather than as an optional struct. Reading nullability from the struct would double-count it and lose the partner's null placement. Spell this out in the javadoc as the one documented exception to the Iceberg-first nullability rule applied by applyCoralNullability. coral-schema suite green: 152 tests, 0 failures.
…ruct encoding @aastha25 asked which of the four uniontype cases are handled. Probing the engine showed only two were: (1) nullable complex partner [null,string,int] -> [null,string,int] ok (2) non-nullable partner [string,int] -> [string,int] ok (3) nullable single partner [null,string] -> [null,{tag,field0}] wrong (4) non-nullable single partner [string] -> [null,{tag,field0}] wrong Cases 3 and 4 leaked the internal Trino {tag, field0} encoding into the output schema, and case 4 additionally invented a null branch the partner never declared. HiveToCoralTypeConverter.convertUnion emits the union-struct encoding for EVERY arity, so uniontype<X> arrives as a 2-field struct, but the memberCount >= 2 guard rejected it and fell through to the struct path. This predates PR4 -- before it, every union-struct was emitted as a record -- so PR4 fixed 1 and 2 and left 3 and 4 behind. The old javadoc calling single unions "vanishingly rare" was wrong: RelDataTypeToHiveTypeStringConverter carries dedicated single-uniontype handling because engines unwrap them and coalesce_struct depends on the distinction. The >= 2 guard existed to avoid misreading a genuine nullable struct that happens to be named {tag, field0}, which also presents one non-null branch. Counts cannot separate those, but the partner's sole branch can: for a genuine struct it is the record describing that struct and so carries its own "tag" field, while for uniontype<X> it is the member type X. Accept single members unless the sole branch describes the struct itself. Renamed isMultiBranchUnionStruct -> isReconstructableUnionStruct since it now also accepts single-branch unions; the predicate form @simbadzina asked for is kept. Five tests added covering all four cases plus both ambiguity directions (genuine {tag, field0} struct stays a record; uniontype<struct<x:int>> whose member record has no tag field is reconstructed) and case-insensitive marker detection. Verified they bite: restoring the memberCount >= 2 behavior fails exactly the three single-union tests. coral-schema suite green: 157 tests, 0 failures.
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
Adds a multi-branch union reconstruction path to
MergeCoralSchemaWithAvro(the Iceberg-first Avro merge engine introduced in #600/#604).Why
Iceberg has no union type. A Hive
uniontype<A,B,C>that has been persisted into an Iceberg table surfaces as the struct{tag:INT, field0:A, field1:B, field2:C}— the Trino union representation (trinodb/trino#3483), the same encodingHiveToCoralTypeConverter.convertUnionproduces — while the partner Avro still describes the column as a real union[null,A,B,C].Before this change, the engine could not match the union partner, so it synthesized a record: multi-branch unions collapsed, and union-typed field defaults failed outright. This diverged from the legacy Hive Avro path (
MergeHiveSchemaWithAvro.union).How
unionPartnerOrNulldetects the case, requiring all of: (1) the Coral struct is union-shaped (tag:INTthenfield0..fieldN-1in order), (2) the partner is an Avro union, and (3) the partner's non-null branch count equals the member count. AmemberCount >= 2guard makes detection collision-free — a genuine nullable struct yields[null, record](one non-null branch), which can never equal a member count of two or more. Single-member union-structs fall back to the struct path.mergeUnionStructmerges eachfieldNmember against the partner union branch by ordinal, strips per-member null wrappers (the union's own NULL branch carries nullability), and emits the NULL branch first when the partner union has one — matchingMergeHiveSchemaWithAvro.union. Emitting a real union also lets the partner's field default apply exactly as on the Hive path.Testing
Six new tests (full
coral-schemasuite green, spotless clean): multi-branch reconstruction, the no-null-branch case, arrays of unions, the union-default regression, and a negative case proving a genuine record partner is not mistaken for a union.