fix: stabilize Vortex runtime for async IO#387
Conversation
020447d to
d0fb7a4
Compare
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for the fix. The runtime isolation approach and the Arrow-side predicate fallback look reasonable, and I verified the focused Vortex tests and clippy locally:
cargo test -p paimon --features vortex arrow::format::vortex -- --nocapturecargo clippy -p paimon --features vortex --all-targets -- -D warnings
I think this needs one fix before merge.
VortexFormatWriter::num_bytes() now returns 0 until close(), because the new writer only updates bytes_written after the whole in-memory Vortex buffer is serialized and written in close(). However DataFileWriter::write and PostponeFileWriter::write rely on current_writer.num_bytes() >= target_file_size after each batch to roll files. With file.format = "vortex", that condition never becomes true before prepare_commit, so target-file-size is ignored and all batches keep accumulating into one open Vortex writer. This is a behavior regression from the previous streaming writer, where CountingPaimonWrite updated bytes_written as Vortex wrote to storage, and it also makes the new in-memory buffering path potentially unbounded.
I reproduced this by adding .option("file.format", "vortex") to test_write_rolling_on_target_file_size: with target-file-size = 1b, the test expects two data files but gets one.
Could you restore observable pre-close progress for Vortex writes, at least enough for target-file-size rolling to work, and add a Vortex-specific rolling test? Tracking queued Arrow/Vortex batch memory as an approximate num_bytes() would be better than always returning zero if exact serialized bytes are unavailable before close().
d0fb7a4 to
5410dbe
Compare
|
Thanks for catching this. I updated the Vortex writer so it now tracks the queued RecordBatch memory estimate while arrays are staged before close. I also added a Vortex-specific regression test for Local verification:
|
QuakeWang
left a comment
There was a problem hiding this comment.
LGTM overall. I left a couple of minor nits, but nothing blocking from my side.
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// Convert a list of Paimon predicates (ANDed together) into a single Vortex filter expression. | ||
| #[cfg(test)] |
There was a problem hiding this comment.
nit: These Vortex expression conversion helpers are now test-only, while the production read path uses Arrow-side filtering. Keeping this conversion test block may make future readers think Vortex filter pushdown is still active. Could we remove it, or rename/comment it as legacy conversion coverage?
| @@ -106,7 +106,7 @@ kanal = { version = "0.1.1", optional = true } | |||
There was a problem hiding this comment.
nit: kanal appears to be unused now that the Vortex writer no longer uses the streaming path. Could we remove it from the vortex feature and the dependency list?
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for the update. I re-reviewed the latest head and the previous blocker is fixed now.
VortexFormatWriter now tracks the staged RecordBatch memory and reports it through num_bytes() / in_progress_size() before close(), so the table writer can observe progress and roll files instead of accumulating everything into a single Vortex writer. The new test_vortex_write_rolling_on_target_file_size covers the regression case I raised.
I also re-ran the focused validation locally:
cargo test -p paimon --features vortex arrow::format::vortex -- --nocapturecargo test -p paimon --features vortex table::table_write::tests::test_vortex_write_rolling_on_target_file_size -- --nocapturecargo clippy -p paimon --features vortex --all-targets -- -D warnings
The full GitHub CI is also green at this head. Looks good to me.
5410dbe to
b2862b8
Compare
|
Addressed the two nits from the latest review:
Local verification:
|
Summary
Stabilizes Paimon's optional Vortex file format on CI without skipping tests. The hang was tied to Vortex 0.68's async scan/write paths and the dependency set that allowed
unicode-segmentation1.13.3, which had already been noted in this crate as correlating with Vortex test hangs.Changes
unicode-segmentationback to=1.13.2, matching the existing comment that 1.13.3 correlates with Vortex test hangs.CurrentThreadRuntimeso returned streams and writers do not depend on the caller's Tokio runtime lifetime.open_buffer, avoiding Vortex's asyncVortexReadAtsegment pipeline for Paimon's storage adapters.OutputFile.Testing
cargo fmt --all -- --checkgit diff --checkcargo test -p paimon --features fulltext,vortex arrow::format::vortex::tests:: -- --test-threads=8 --nocapturecargo test -p paimon --all-targets --features fulltext,vortexcargo test -p paimon-datafusion --features vortex --test vortex_tablescargo clippy --all-targets --workspace --features fulltext,vortex -- -D warnings27588470873passed: check, unit/build on Ubuntu/macOS/Windows, and integration.Notes
This does not skip any Vortex tests. The trade-off is that Vortex reads/writes are buffered in memory while Vortex support remains optional and experimental; this favors deterministic CI behavior over streaming/random IO for now.