[common] Keep the selected variant schema when its evidence is gone - #9532
[common] Keep the selected variant schema when its evidence is gone#9532PDGGK wants to merge 3 commits into
Conversation
finalizeAdaptiveSchema falls back to the previously selected schema when a node has no evidence in the current file, then runs it through the RowType branch as if it were evidence. Selected schemas carry no per-field counts, so getFieldCount throws IllegalStateException and the whole file fails to write. Reaching it needs a node that drifted across a type family, which degrades its combined evidence to VARIANT while its selected schema is a ROW, and that is then absent from the next file. Return the selected schema as it is instead: it is already a finalized selection and has nothing left to threshold.
| // not | ||
| // evidence - its fields carry no counts - so it cannot be run through admission and | ||
| // retention again; carry it forward unchanged. | ||
| return previousSelected; |
There was a problem hiding this comment.
[P2] Count preserved descendants against the shared width budget
maxFields is shared across every Variant path in this inference, but this early return only consumes the single unit decremented at method entry. A preserved RowType/ArrayType can contain descendants that would normally consume additional units, so later Variant columns can now exceed variant.shredding.maxSchemaWidth.
I reproduced this with maxSchemaWidth = 8 and two Variant columns: the first evolves from p: scalar to p: {x: scalar} and then omits p, while the second adds a new r field in the third round. This branch carries ROW<x BIGINT> forward without accounting for x, then also selects q and r in the second column, using 9 budget units. A focused assertion that r remains untyped fails because it becomes BIGINT.
Please preserve the selected schema while also walking/debiting its nested width (and handling insufficient remaining budget consistently) before returning.
Returning the selected schema early only spent the single unit taken at method entry, so its descendants occupied width that maxSchemaWidth never counted and a later variant column could overspend the shared budget. Walk the retained schema instead, charging one unit per node and dropping what no longer fits, which is what the evidence-driven walk does.
| List<DataField> fields = new ArrayList<>(); | ||
| for (DataField field : ((RowType) selected).getFields()) { | ||
| maxFields.remaining--; | ||
| if (maxFields.remaining <= 0) { |
There was a problem hiding this comment.
[P2] Preserve the retained field/container at the last budget unit
This does not match the evidence-driven walk described above. When one unit remains, that walk enters the child, decrements the unit to zero, returns VARIANT, and still adds the parent DataField; this helper decrements first and breaks before adding it. If it was the only field, line 678 then collapses the whole retained RowType to the root VARIANT. The Array branch similarly collapses the array instead of retaining ARRAY<VARIANT>.
I reproduced this with maxSchemaWidth = 7 and two Variant columns. Round 1 is (1, 5), round 2 is (1, {q:1}), and round 3 is ({x:1,y:1}, null). Column a consumes five units, leaving exactly the root plus one field unit for retained column b. The updated code returns an untyped root for b and loses q; a focused assertion that b still contains q fails.
Please mirror finalizeAdaptiveSchema at exhaustion: retain the row field/array container and downgrade the exhausted child to VARIANT, rather than breaking/returning before preserving the parent shape.
The helper decremented before its guard, so the field that consumed the final unit was dropped rather than kept with a VARIANT child, and a retained node holding only that field collapsed entirely. Enter a child only while budget remains, spend the unit on entry, and downgrade an exhausted child to VARIANT with its field or array container intact, which is what the evidence-driven walk does.
|
Both P2s confirmed and fixed in Width budget. With Exhaustion semantics. Your second point was right about my first attempt: it decremented before its guard, so the field that consumed the final unit was dropped rather than kept with a Three controls, so each test is pinned to the thing it covers rather than to the crash:
|
| ((ArrayType) selected).getElementType(), maxFields); | ||
| return new ArrayType(element); | ||
| } | ||
| maxFields.remaining--; |
There was a problem hiding this comment.
[P2] Do not charge retained VARIANT leaves twice
The caller already spends the node entry unit before retainSelectedSchema is reached. For a VariantType, there is no typed-value child to spend another unit on; the normal finalizeAdaptiveSchema path likewise returns VARIANT after only the entry debit. This branch therefore overcharges retained VARIANT leaves.
A minimal boundary case reproduces the regression: use two top-level variant columns with maxSchemaWidth = 3, infer/commit [null, 5], then infer [null, 6]. The first retained VARIANT should cost one unit and leave two for the second BIGINT, but this extra decrement exhausts the budget and the second column becomes untyped VARIANT. I confirmed that assertion fails against this head while the existing 27 tests pass. Please special-case VariantType here and add the shared-budget regression.
2 participants
Purpose
Adaptive variant shredding inference throws and fails the write when a node's evidence is gone but its selected schema is still a
ROW.finalizeAdaptiveSchemasubstitutes the previously selected schema when the current file has no evidence for a node:Execution then falls into the
RowTypebranch, which reads that substitute as if it were evidence:Selected schemas carry no per-field counts —
finalizeSimpleSchemaclears them, and the adaptiveRowTypebranch rebuilds fields with the description-free three-argumentDataFieldconstructor — sogetFieldCountthrows:Getting there needs three things in one rolling writer with
variant.inferShreddingSchema=trueandvariant.shredding.inferenceMode=adaptive: a node drifts across a type family, which degrades its combined evidence toVARIANTwhile the schema selected for it is aROW; and it is then absent from the next file, so there is no current evidence either. Two files cannot do it — when a node is absent,combineEvidencefalls back to the previous file's evidence, which still carries counts — so it takes three.The exception comes out of
InferShreddingWritePlanWriteruncaught, so the file fails to write at all.Three files of a
ROW<v VARIANT>table, at the default options, are enough:{"k":1,"p":5}pselected asBIGINT{"k":1,"p":{"x":1}}pselected asROW<x BIGINT>, combined evidence forpdegrades toVARIANT{"k":1}A root-level variant of the same shape —
42, then{"a":1}, then SQLNULL— fails the same way withField 'a'.Summary and Changelog
Return
previousSelectedinstead of assigning it tocombined. It is already a finalized selection: it has been through admission, retention and the field budget once, and with no evidence for this file there is nothing to re-threshold it against. Carrying it forward unchanged is also what the retention path does for a node whose evidence is still aRowType.Tests
InferVariantShreddingSchemaTest#testAdaptiveInferenceKeepsSelectedRowWhenEvidenceDegradedAndNodeIsAbsentdrives the three files above and assertspkeepsROW<x BIGINT>. Onmasterit errors with theIllegalStateExceptionabove, and the other 24 cases in the class are unaffected either way.Four negative controls, run against a build of this ref, confirm the trigger is that specific combination rather than absence alone — no drift and
pmerely disappears; drift butpstill present in file 3; object all the way, which is the shape every existing adaptive test uses; and root scalar to scalar to NULL. All four pass before and after.The reason no existing test catches this: the four adaptive cases in
InferVariantShreddingSchemaTestandInferVariantShreddingWriteTestall keep the root variant an object across every file, so combined evidence stays aRowTypeand line 577 is never reached with aRowTypeinpreviousSelected.testAdaptiveInferenceWidensScalarSelectedFromPriorEvidencecomes closest — itssecondcolumn has no current evidence in round 2 — but its combined evidence there is the previous file's evidence, which still carries counts.mvn test -pl paimon-common -Dtest='org.apache.paimon.data.variant.**'— 52 tests, and-pl paimon-format -Dtest=InferVariantShreddingWriteTest— 18 tests, all passing.spotless:checkandcheckstyle:checkclean.Tests API and Compatibility
No API, format or configuration change. Only the path that currently throws behaves differently; every schema that infers successfully today infers the same way.