fix: align byte-sized batches across data files - #8189
Conversation
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The multi-file boundary stitching fixes the reported crash, but strict batching still overrides the advertised combined row/byte contract after the read and can rebuild batches beyond the byte cap. Reject the conflicting strict_batch_size combination or define a precedence rule and enforce it in the final output plan.
| /// When set, the scanner will produce batches whose total size in bytes | ||
| /// is approximately this value, overriding the row-based `batch_size`. | ||
| /// is approximately this value. When a row-based `batch_size` is also set, | ||
| /// both limits apply and the one reached first determines the batch size. |
There was a problem hiding this comment.
strict_batch_size(true) still makes this promise false: StrictBatchSizeExec is applied after the byte rechunker and concatenates batches until batch_size, so a large row target can recreate the allocation pressure the byte limit is meant to bound.
I extended test_batch_size_bytes_across_data_files on this head with:
scan.project(&["id", "wide"])
.unwrap()
.batch_size(200)
.batch_size_bytes(8 * 1024)
.strict_batch_size(true);and reused its logical-byte assertion. The first output batch was 14,404 bytes for an 8,192-byte target. Please either reject this incompatible option combination at plan construction, or define which contract wins and enforce that choice; add this case to the regression.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The follow-up rejects strict row batching with any effective byte limit at plan construction, resolving the prior memory-bound violation while preserving the verified multi-file alignment fix. An explicit incompatibility is preferable here because strict row counts and byte-capped output cannot both be guaranteed.
westonpace
left a comment
There was a problem hiding this comment.
This introduces a copy and it doesn't solve the problem that each data file will read its own batch_size_bytes limit. For example, if the user sets batch_size_bytes to 1MB and there are 100 data files in a fragment then each batch could fetch up to 100MB of data.
However, it does address the "what to do if both batch_size and batch_size_bytes" are set and it goes from an error to a data copy in the multi-data-file case so I think it is at least a step in the right direction.
Summary
Root cause
Each data-file reader selected its own rows-per-batch from the byte target. Mixed-width files therefore emitted different batch boundaries, while
merge_streamsassumed equal row counts and attempted an invalid column merge. The decoder also treated the byte setting as a replacement for the row limit.Validation
cargo test -p lance-table test_zip_with_different_batch_boundariescargo test -p lance-encoding test_byte_sized_batches_respect_row_limitcargo test -p lance test_batch_size_bytes_across_data_filescargo fmt --all -- --checkcargo clippy --all --tests --benches -- -D warningsmake buildfrompython/uv run make lintfrompython/(Pyright completed with 10 missing-third-party-stub warnings and no errors)Environment note:
make installbuilt and installed the local extension, then its pre-commit hook step was blocked because this automation checkout setscore.hooksPath=/dev/null. The required Python build and lint commands above completed successfully afterward.Fixes #8099