fix(core): take the merge-on-read schema from the base file - #665
Open
linliu-code wants to merge 4 commits into
Open
fix(core): take the merge-on-read schema from the base file#665linliu-code wants to merge 4 commits into
linliu-code wants to merge 4 commits into
Conversation
Squashed view of apache#639-apache#662 for review. Not for merge — the reviewable increments are those PRs; this is the same code in one diff. Ports the merge-on-read file group reader from onehouseinc/hudi-rs-internal into hudi-core, wires it behind a switch that defaults to the reader that has always served reads, and brings its end-to-end test harness across. The ported reader is `pub(crate)` and reached only through `hoodie.read.merge.engine = v2`. Nothing changes for anyone who does not set it. It is not at parity yet: the gaps are pinned as ignored cases carrying their findings, and the outstanding decisions are in the PR descriptions. Four fixes land on paths the existing reader shares: decimals had no Arrow conversion, Avro timestamp logical types lost their UTC zone, the properties-escaped create schema was not being unescaped by one of its two consumers, and the base file reader had no way to accept a pushdown predicate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A delete block whose ordering value is anything but a small integer fails to
read, with errors like `Union index 1490 out of bounds: 2`. The index is
nonsense because the byte stream is misaligned, not because a branch was chosen
wrongly.
`orderingVal` is declared here as a union of primitives. Hudi writes a union of
per-type wrapper records, and inserted `BooleanWrapper` at position 1, so every
position from `int` onward names a different type than this crate assumes.
Position 3 is `float` here and `LongWrapper` there. Reading a long as a float
consumes four bytes instead of two, and the next record's key length is read
from the middle of the previous value.
The delete block in `table_delete_ord_long` is exactly self-consistent under
Hudi's schema and not under this one:
04 | 02 02 34 | 02 00 | 06 c0 3e | 02 02 33 | 02 00 | 06 f0 2e | 00
two records, keys "4" and "3", ordering position 3, values 4000 and 3000
Decoded here, position 3 is a float, so `c0 3e 02 02` is eaten and `33` becomes
the next key's union index: zigzag 0x33 is -26, which is the reported error.
So this takes Hudi's schema. Two things follow from it:
The Arrow side wants a scalar, not a record with one field, so the wrapper is
unwrapped after the schema is narrowed — narrowing reads the position Hudi
wrote, and unwrapping rewrites it, so the order matters.
The wrapper is chosen for the value rather than for the column, so a table
whose ordering column is a long can still carry an `IntWrapper` for a small
value. The delete batch's ordering column is now cast to the type the data
schema declares. The old schema hid this by calling position 2 a long
regardless, which happened to match the two fixtures that exercise it.
`ArrayWrapper` orders by a list and is rejected in both places that read the
position, rather than mapped to something that would disagree with its value.
Older tables written with the primitive union are not supported. No fixture
here uses one, and the two shapes cannot be told apart from the bytes: where
they differ, decoding usually fails, and at position 2 both succeed and yield
the same number.
Un-ignores the eleven cases that pinned this.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An Avro map was modelled as `Dictionary(Utf8, V)`. An Arrow dictionary key must be an integer, so that is not a valid type — and it does not reconcile against the `Map` a parquet base file carries, so any table with a map column fails to read once a log block has to be merged with its base file. Avro maps become `Map(key_value: struct<key: string, value: V>)`, matching what the parquet reader produces, so the two agree by name and by shape. The array side builds a `MapArray`. Entries are materialized as two-field records so the existing struct machinery builds both children, which is also why `child_schema_lookup` now registers those two positions — a struct-valued map needs its own fields resolvable underneath them. Entries are emitted in key order. Avro maps are unordered and the Arrow type says so, but a stable order keeps a read reproducible rather than dependent on hash iteration. This is on the shared conversion, so it fixes the existing read path too: a map column has never been readable through either reader. Un-ignores the case covering NULL elements inside containers. Two other cases that were pinned on this stay pinned, now on a decimal column reading as NULL — a separate gap this one was masking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The merge-on-read reader was given its schema from `hoodie.table.create.schema`. That is the schema the table was created with, and Hudi treats it as a last resort: `TableSchemaResolver` reads the latest commit's metadata first, then a base file's footer, and only then falls back to the create schema. This crate's own `schema::resolver::resolve_data_schema` does the same. The reader was reaching past both for the weakest source. Reading the base file's own schema is what the existing path effectively does, so the two engines now start from the same types. It is also what the data actually has: under schema evolution the create schema is stale, and the engine evolves each batch to the required schema regardless. Three workarounds go with it. The create schema arrives as Java writes a properties file, with `:` escaped, so it had to be unescaped before it would parse as JSON. It carries no `_hoodie_*` columns, so those had to be prepended when the table populates them. And a table that never recorded one could not be read at all — which included every reader built from a bare base URI, the shape the cxx bridge uses. Slices with no log files now go through the engine too. They were held back because the create schema modelled a map as an invalid Arrow dictionary and every fixture here has a map column; the schema no longer comes from there, and the conversion itself is fixed separately. The engine reduces to a base file read, and the test asserting that the setting does not change such a read now compares the two engines rather than one path with itself. Reading the footer costs one request. The engine reads it again when it opens the file; collapsing the two is worth doing but is not this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Stacked on #639–#664 — review only the last commit.
What was wrong
The merge-on-read reader took its schema from
hoodie.table.create.schema— the schema the table was created with.Hudi treats that as a last resort.
TableSchemaResolverresolves in this order:hoodie.table.create.schemaAnd this crate already implements the same order in
schema::resolver::resolve_data_schema. The reader was reaching past both better sources for the weakest one.What it does now
Reads the base file's own schema — Hudi's tier 2, and what the existing read path effectively uses, so the two engines now start from the same types. It is also what the data actually has: under schema evolution the create schema is stale, and the engine evolves each batch to the required schema regardless.
Three workarounds removed
Each of these existed only to make the create schema usable:
\:_hoodie_*The third is the one that mattered: it was a standing gap for the standalone entry point, and it closes here without new plumbing, because every read already has a base file path.
Base-file-only slices now route through the engine
They were held back in #659 because the create schema modelled an Avro map as
Dictionary(Utf8, V)— an invalid Arrow type — and every parquet fixture here has a map column. The schema no longer comes from there, and the conversion itself is fixed in #664, so the guard is gone.The engine reduces to a base file read for these. The test asserting that setting
v2does not change such a read now genuinely compares the two engines rather than one path against itself.Cost
One footer request. The engine reads the footer again when it opens the file; collapsing the two is worth doing and is not this change.
Tests
No new ones — this removes special-case handling rather than adding behavior, and the existing coverage tightens as a result: the base-file-only comparison becomes a real differential test, and every harness case that reads a slice now exercises the base-file schema path.
Full workspace green: 1180 lib + 79 table-read + 39 datafusion + 21 + 12.
🤖 Generated with Claude Code