fix(trino): read LSM archived-timeline parquet files through a Trino-… - #19732
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19732 +/- ##
============================================
- Coverage 77.96% 75.99% -1.98%
+ Complexity 33458 32775 -683
============================================
Files 2539 2539
Lines 140939 141359 +420
Branches 17012 17076 +64
============================================
- Hits 109890 107428 -2462
- Misses 23388 26380 +2992
+ Partials 7661 7551 -110
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@wombatu-kun Could you please review this fix too? |
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR replaces the UnsupportedOperationException in HudiTrinoFileReaderFactory.newParquetFileReader with a real TrinoParquetFileReader that reads LSM archived-timeline Parquet files through Trino's ParquetReader, so Trino reads that consult the archived timeline (e.g. slicing a MOR file group whose log file belongs to an archived delta commit) no longer fail. I traced the data path end to end: the SqlVarbinary to ByteBuffer conversion matches how ArchivedTimelineV2 casts metadata/plan, the projected schema is mapped by name (FULL's reversed plan/metadata order is handled), all reachable read schemas use only string/bytes types the reader supports, resources are released on both success and failure paths, and the change is confined to the archived-timeline read path with no other callers of the factory's parquet path. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
…backed reader HudiTrinoFileReaderFactory.newParquetFileReader threw UnsupportedOperationException, and hudi-common's ArchivedTimelineLoaderV2 obtains its reader for the LSM history files under .hoodie/timeline/history from that factory. Since apache#14019 the archived timeline is loaded lazily, so the throw surfaces whenever a completion time of an archived instant is needed, e.g. file slicing of a MOR table whose log file belongs to an archived delta commit. Add TrinoParquetFileReader, a HoodieAvroFileReader over Trino's ParquetReader that turns each page into an IndexedRecord through HudiAvroSerializer, honouring the projection the loader requests and converting VARBINARY values to the ByteBuffer that ArchivedTimelineV2 expects for the metadata and plan columns. Key, key-prefix and row-key lookups are unsupported: timeline files carry none of the data-file footer metadata those rely on. Ported from onehouseinc/trino#72 and adapted to the HoodieSchema-based reader contract, the PrefilledColumnValues serializer and Trino 484's ParquetReader API. Fixtures: a four-instant history file for the reader unit test and tv8 COW/MOR tables with archived timelines for the smoke test; the MOR fixture's oldest log file belongs to an archived delta commit, so its read-optimized and real-time cases fail on master and pass with this change. Fixes apache#13994
- Stop re-entering ParquetReader.nextPage() after it returned null: on Trino 484 a second nextBatch() past the last row group throws IndexOutOfBoundsException, so the iterator now remembers exhaustion; the empty-page loop is gone (nextBatch never yields an empty page). - close() closes every iterator the reader handed out, matching HoodieAvroParquetReader. - The footer-only open no longer passes the file size, which made createDataSource pick the MemoryParquetDataSource and read the whole file once for the footer and again for the data. - readMinMaxRecordKeys()/readBloomFilter() throw UnsupportedOperationException like the other key lookups; no Trino path reaches them and re-reading the footer per call was wasteful. - Tests: the projection test runs LoadMode.TIME and FULL (FULL orders plan before metadata, the reverse of the file, so it proves name-based mapping and pins the ByteBuffer conversion and the null-bytes arm); the footer test asserts the lookups are unsupported; the schema assertion compares against HoodieLSMTimelineInstant; package-private test class. - Drop the COW smoke case and fixture: with the lazy archived load it passes without the fix (no data log files, the metadata-table view failure is caught and falls back to listing), so it guarded nothing for a 366 KB zip inflated by every query-runner class. The reader unit fixture, which is that table's history file, keeps its provenance in the test javadoc. - MOR fixture doc: ASF header (validate-source RAT failed without it), the Hudi version the table was written with, and drop a tblproperty that does not exist.
- trinoStorage is a constructor local, not a field - restore hudi_cow_archived_timeline.md (script only) and point the test javadoc at it: archived_timeline.parquet is that table's first LSM history file - cover the drained-iterator and close-with-open-iterator guards
8fcdd4b to
592d51f
Compare
…and PLAN reads The helpers under TrinoParquetFileReader took the requested projection under the name readerSchema, next to a MessageType local called requestedSchema holding the same projection. Rename the Avro parameter to projectedSchema in buildTrinoColumns, buildColumnHandles, binaryFieldPositions, createParquetReader and the iterator constructor, and fix the loadNextPage comment to name nextPage(), the call it sits on, rather than the private nextBatch(). The projected-read test only ran TIME and FULL, and FULL is requested by nothing but EightToSevenDowngradeHandler. METADATA and PLAN are the modes whose bytes column ArchivedTimelineV2 casts to ByteBuffer, so add both to the EnumSource and check the metadata and plan columns under separate guards, since each of those two modes carries only its own column.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR adds a TrinoParquetFileReader so the Trino connector can read LSM archived-timeline parquet files (fixing #13994), wiring it into HudiTrinoFileReaderFactory.newParquetFileReader. I traced the serializer type contract, channel/field-by-name alignment (including the FULL reversed-column case), schema coverage against the LSM instant schema and all read projections, the ArchivedTimelineLoaderV2 integration (per-file reader in a parallel stream, iterator-level resource release), and the iterator state machine. No new issues surfaced beyond the items already raised in prior rounds. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. One naming nit on the exception-wrapping helper.
| } | ||
|
|
||
| private static TrinoException handleException(StoragePath path, Exception exception) | ||
| { |
There was a problem hiding this comment.
🤖 nit: could you rename handleException to wrapException (or toTrinoException)? "handle" conventionally implies the method deals with or suppresses the exception, but here it constructs and returns a new TrinoException for the caller to throw — the current name causes a double-take at every call site.
There was a problem hiding this comment.
Keeping the name: it mirrors Trino's ParquetPageSource.handleException, which has the same signature and body and is handed to ParquetReader in the same exception-transform slot, so it reads the same as the Hive connector code it sits next to.
…leException, type handles with HudiUtil TrinoParquetFileReader#close stopped at the first iterator whose close() threw, leaving the rest open and the list unclear. It now closes them all, rethrows the first failure and attaches the later ones as suppressed. The constructor let the IOException of a footer read escape raw, so a ParquetCorruptionException reached hudi-common as a HoodieIOException instead of the HUDI_BAD_DATA TrinoException a failed page read produces. The open path (file length and footer) and createParquetReader, data-source open included, now throw through handleException; the constructor no longer declares IOException and the factory's catch that wrapped it is gone. A test builds the reader over a three-byte file and expects HUDI_BAD_DATA with the corruption as its cause. The hand-rolled Avro-to-Trino type switch and the HiveColumnHandle loop duplicated HudiUtil.toColumnHandle, so the iterator now takes the HoodieSchema projection and builds its handles with it; the Trino columns and the VARBINARY positions come off those handles. testReadTableWithArchivedTimeline takes its RO/RT flag from a ValueSource and names the MOR table in its body; the provider that emitted the same table twice is gone. The COW create script's comment now names .hoodie/timeline/history, the directory the line under it checks.
…s() throws IOException
…tions uses, not its column index
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR adds TrinoParquetFileReader so Trino can read LSM archived-timeline parquet files through Trino's ParquetReader, replacing the UnsupportedOperationException in HudiTrinoFileReaderFactory#newParquetFileReader that surfaced whenever an archived instant's completion time was needed. I traced the reader against its real consumers in hudi-common (ArchivedTimelineLoaderV2, CompletionTimeQueryViewV2, ArchivedTimelineV2) — the unsupported-method throws are genuinely unreachable (only LoadMode.TIME via the two-arg getIndexedRecordIterator), resource ownership is sound (the iterator owns and closes the ParquetReader; the constructor holds nothing open), and the SqlVarbinary → ByteBuffer.wrap conversion lines up with ArchivedTimelineV2's .array() read. No new issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
…handleException The iterator of TrinoParquetFileReader caught only IOException around loadNextPage(), but the page is decoded eagerly there and a data page that fails to decode surfaces as a ParquetDecodingException, which is unchecked: ParquetReader routes only the IOExceptions of a page read through the exception transform it is handed. The raw exception passed through hudi-common untouched and reached the engine as an internal error. hasNext() now catches IOException and RuntimeException alike, as createParquetReader does, so the failure is a HUDI_CURSOR_ERROR TrinoException with the cause attached. A test flips the encoding byte of the fixture's first data page from PLAIN to RLE_DICTIONARY, so the page claims a dictionary its column chunk does not carry, and expects HUDI_CURSOR_ERROR with the ParquetDecodingException as its cause. Without the wider catch the raw exception escapes hasNext() and the test fails.
5 participants
…backed reader
Describe the issue this Pull Request addresses
Fixes #13994.
HudiTrinoFileReaderFactory.newParquetFileReaderthrowsUnsupportedOperationException, and hudi-common'sArchivedTimelineLoaderV2gets its reader for the LSM history files under.hoodie/timeline/historyfrom that factory. Since #14019 the load is lazy, so the throw surfaces whenever an archived instant's completion time is needed, e.g. slicing a MOR file group whose log file belongs to an archived delta commit.Summary and Changelog
TrinoParquetFileReader: aHoodieAvroFileReaderover Trino'sParquetReader, converting pages toIndexedRecords viaHudiAvroSerializerwith the loader's projection. VARBINARY values become theByteBufferthatArchivedTimelineV2castsmetadata/planto. Key, bloom-filter and min/max-key lookups throw; no Trino path needs them.HudiTrinoFileReaderFactory.newParquetFileReaderreturns that reader.HoodieSchemareader contract,PrefilledColumnValuesand Trino 484'sParquetReaderAPI.Impact
Trino reads that consult the archived timeline no longer fail. No config or API change. Only
LoadMode.TIMEis reachable from the connector today.Risk Level
Low, confined to the archived-timeline read path.
TestTrinoParquetFileReader: full read,TIMEandFULLprojections (FULLordersplanbeforemetadata, the reverse of the file, so it pins name-based mapping and theByteBufferconversion), unsupported footer lookups.TestHudiSmokeTest.testReadTableWithArchivedTimeline: a tv8 MOR fixture whose oldest log file belongs to an archived delta commit, read-optimized and real-time; both cases fail on master. tv8 is representative: the LSM instant schema is unchanged since #9209.Documentation Update
none
Contributor's checklist