chore(core): prevent table suspension on out-of-order inserts into parquet partitions - #7310
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/azp run macwin |
|
Azure Pipelines successfully started running 1 pipeline(s). |
[PR Coverage check]😍 pass : 27 / 27 (100.00%) file detail
|
bluestreak01
left a comment
There was a problem hiding this comment.
Approving after a level-3 review.
The fix correctly repairs a real, deterministic regression (introduced by #7107) that suspends tables on out-of-order inserts into parquet partitions. Verified end-to-end:
- Empty window now passes primary_data=null, size=0, clearing the Rust
from_raw_datanull-with-non-zero-size guard. - Non-empty window hands the encoder
addressOf(dataLo)+ window-relative aux rebased via the existingshiftCopyAuxVectorprimitive; rebased offsets are bounded by windowSize, so every encoder bounds guard (VARCHAR/BINARY/STRING errors, ARRAY assert) holds at the boundary. Confirmed for all four var-size drivers. - The 0-based copy+sort path is byte-for-byte unchanged;
MemoryCARWis not assignable toMemoryOM, so the guard cleanly partitions the two paths. - Scratch arena is sized once up front (stable slot addresses), lazy-allocated, freed in close() and on CarrierLocal teardown; per-carrier-thread so no concurrency/reentrancy.
populateO3DescriptorColumns is private static with exactly two callers, both updated, and the new getter is internal-only, so the cross-context blast radius is zero.
The tests are strong and honest: strict builder API throughout (no banned assertSql, no returnsOnce), all four drivers, empty/non-empty windows, mixed varchar shapes, three-driver arena packing, and two-partition fan-out. Three are real master-failing repros; the six round-trip tests are candidly labelled as guards.
Non-blocking nits:
- Title: this is a user-impacting regression fix, so
fix(core):reads better in release notes thanchore(core):(Bug/regression labels already reflect this). - The column-top var-column path is reachable but untested (acknowledged in the javadoc); a follow-up regression test would close the only material coverage gap.
- The rebase arena never shrinks, so a single large block apply inflates each participating carrier thread's native footprint until shutdown - consistent with other reusable O3 buffers and disclosed in the PR.
Summary
An out-of-order insert whose rows sort into an existing parquet partition could suspend the table with a native error:
It is deterministic, not load-dependent, and reproducible with pure SQL: convert a partition to parquet, then apply an in-order block of WAL rows that sort before the partition's existing data while a var-size column (VARCHAR/STRING/BINARY/array) holds long, non-inlined values. The fix corrects the var-column pointers that the optimised WAL block-apply path hands the parquet writer, so the apply completes and the table stays available.
Root cause
When a WAL commit takes the optimised "all data in order, single segment" block path,
TableWriter.processWalCommitBlockmaps the WAL segment columns directly instead of copying them into a fresh 0-based buffer, and sets the O3 row offset to the block's segment offset (segmentCopyInfo.getRowLo(0)). That offset is non-zero once the segment already carries rows from an earlier block. For a var-size column the data file is then offset-mapped over the window[dataOffset(rowLo), dataOffset(rowHi)), soMemoryCMORImpl.addressOf(0)returns a linear extrapolation to "where absolute byte 0 would be", not the start of the mapped window.The parquet O3 path (
O3PartitionJob.populateO3DescriptorColumns, used by both the COPY_O3 append and the fresh-parquet write) built the var-column descriptor withprimary_data = addressOf(0)and an absolute data size, and the Rust encoder then constructed a slice[addressOf(0), addressOf(0) + dataExtent)indexed by absolute aux offsets. That slice spans the unmapped[0, dataOffset(rowLo))prefix:addressOf(0) == 0, and the native null-pointer guard throws and suspends the table (the case above).The sibling MERGE path is unaffected: it feeds the windowed buffers to
Vect.*merge functions that computepointer + offsetdirectly and write a dense, 0-based result before handing it to the encoder. The normal copy+sort apply path is also unaffected: it produces a 0-based buffer whereaddressOf(0)is the real base.The fix
populateO3DescriptorColumnsnow detects a windowed source (oooDataMem instanceof MemoryOM) and, for it, hands the encoder the real data window —addressOf(dataLo), or null when the window is empty — together with a window-relative aux rebased via the existingColumnTypeDriver.shiftCopyAuxVectorprimitive (dest = src - dataLo). The rebased aux for each var column lives in a reusable scratch arena added toO3ParquetMergeContext, sized once per apply so the per-column slots keep stable addresses across the column loop. The 0-based copy+sort path keeps its existing behaviour exactly.Both parquet O3 writers funnel through this one method, so the single change covers the COPY_O3 append and the fresh-parquet write. There is no Rust/JNI change: the encoder's contract becomes "aux offsets are relative to
primary_data", which it already satisfies.Scope and tradeoffs
MemoryOM) rather than the offset value, so it touches no currently-working normal-path write.O3ParquetMergeContextgains a native scratch buffer (16 KiB initial, grown on demand, freed on context close) for the rebased aux.Test plan
New
WalParquetO3BlockApplyTestcovers the windowed-source rebase through bothpopulateO3DescriptorColumnscallers. Each test converts a partition to parquet (or, for the fresh-parquet case, creates aFORMAT PARQUETtable) and applies an in-order O3 block that sorts before existing data, so the var columns reach the encoder as offset-mapped WAL segments.Three tests are deterministic suspension repros that fail on master and pass with the fix. Each leaves the var column null in the trigger block, so the data window is empty (
addressOf(0) == 0) with a non-zero absolutedataExtent:testInOrderO3IntoParquetPartitionViaBlockApply— all-null VARCHAR, the COPY_O3 callertestInOrderO3IntoParquetWithNullArrayColumn— all-null DOUBLE[], the COPY_O3 caller, exercisingArrayTypeDriver's distinctgetDataVectorOffset/shiftCopyAuxVectortestInOrderO3IntoFreshParquetPartitionViaBlockApply— all-null VARCHAR born into a freshFORMAT PARQUETpartition, thewriteFreshParquetFromO3callerSix tests pin the non-empty-window data correctness (real data read through
addressOf(dataLo)plus rebased offsets). They pass with or without the fix — for a non-empty window the pre-fix extrapolation still resolves back inside the mapped region — so they are round-trip guards, not crash repros:testInOrderO3IntoParquetWithNonNullVarSizeColumns— VARCHAR + STRING, asserting element-for-element round-triptestInOrderO3IntoParquetWithBinaryColumn— BINARY (BinaryTypeDriver, aStringTypeDriversubclass)testInOrderO3IntoParquetWithArrayColumn— non-null DOUBLE[]testInOrderO3IntoParquetWithMixedVarcharWindow— null, inlined, and long VARCHAR in one blocktestInOrderO3IntoParquetWithThreeVarSizeDrivers— STRING + VARCHAR + DOUBLE[] coexisting in the scratch arenatestInOrderO3IntoTwoParquetPartitions— one block fanning into two parquet partitionsThe suite cannot assert directly that the optimised single-segment block path ran — that would need a production seam — so the setup forces it and the COPY_O3 row-group delta (or, for the fresh-parquet case, the born-parquet format) is the proxy. A future heuristic that stopped selecting the block path would make the suspension repros pass vacuously.
Existing suites pass:
O3ParquetMergeStrategyFuzzTest,AlterTableConvertPartitionTest,O3ParquetStaleReaderTest.Run: