[rust] make scanning in order configurable - #528
Merged
Conversation
changhiskhan
force-pushed
the
changhiskhan/limit-ordered
branch
from
February 6, 2023 03:09
00aae52 to
9ce95db
Compare
eddyxu
reviewed
Feb 6, 2023
| let stream = stream::iter(0..reader.num_batches()) | ||
| .map(|batch_id| async move { r.read_batch(batch_id as i32, ..).await }); | ||
|
|
||
| if scan_ordered { |
Member
There was a problem hiding this comment.
let stream = if scan_order {
stream.buffered(...)
} else {
stream.buffer_unorderred(...)
}
stream.try_for_each(....) Simplify a bit?
| manifest.clone(), | ||
| PREFECTH_SIZE, | ||
| true, | ||
| false, |
Member
There was a problem hiding this comment.
I am not sure that how much difference it is before ordered / unordered at the moment. Should we just implement ordered scan, if later we find that there are meat here to change to unordered, by that time, we add this flag later?
changhiskhan
force-pushed
the
changhiskhan/limit-ordered
branch
from
February 6, 2023 04:00
9ce95db to
207f2b9
Compare
eddyxu
approved these changes
Feb 6, 2023
Xuanwo
pushed a commit
that referenced
this pull request
Aug 10, 2026
…oto (#7432) Fixes: #7080 ## Summary Follow-up to #6650. The `updated_fragment_offsets` field (proto field 9) stores per-fragment matched row offsets as `map<uint64, UInt32List>` -- one uint32 per matched row. For dense rewrites this produces multi-GB manifests (e.g. 86k matched rows x 4 bytes x many fragments). This PR adds proto field 10 (`map<uint64, bytes>`) using portable RoaringBitmap serialization, which typically compresses the same data to tens of bytes per fragment. Writers emit field 10 only; readers prefer field 10, falling back to field 9 for manifests written before this change. ## Background PR #6650 added `updated_fragment_offsets` to the `Update` transaction message so that `build_manifest` can partially refresh `_row_last_updated_at_version` for matched rows only. The encoding choice -- one uint32 per offset in a `UInt32List` -- was flagged post-merge as a size regression for dense updates. The offsets are already stored internally as `RoaringBitmap`; this PR aligns the proto encoding with that representation. ## Changes ### protos/transaction.proto - Deprecate field 9 (`map<uint64, UInt32List> updated_fragment_offsets`) with a comment pointing to field 10. - Add field 10: `map<uint64, bytes> updated_fragment_offset_bitmaps` with documentation of the dual-read strategy. ### rust/lance/src/dataset/transaction.rs Serialization (`From<&Transaction> for pb::Transaction`): - Write field 10 only: `RoaringBitmap::serialize_into` produces portable bytes for each fragment's bitmap. - Set field 9 to an empty `HashMap` (forward compat; old readers ignore unknown fields). Deserialization (`TryFrom<pb::Transaction> for Transaction`): - If field 10 is non-empty: deserialize each entry with `RoaringBitmap::deserialize_from`. - Else if field 9 is non-empty: convert each `UInt32List` to `RoaringBitmap::from_iter` (legacy fallback). - Same `if !new_field.is_empty() { ... } else { ... }` pattern used by the existing `Rewrite.groups` / `Rewrite.old_fragments` migration. Invalid field 10 bytes fail deserialize with `Error::invalid_input`. In-memory type unchanged: `UpdatedFragmentOffsets(HashMap<u64, RoaringBitmap>)`. ## Test plan - `test_proto_round_trip_field_10` -- write a transaction with field 10, read back, verify offsets match for two fragments. - `test_proto_legacy_field_9_read` -- construct a proto with only field 9 populated (simulating an old writer), deserialize, verify offsets are correctly recovered. - `test_proto_field_10_takes_precedence_over_field_9` -- when both fields are present, field 10 values are used and field 9 is ignored. Proto wire format change; team vote may be needed. ## Backward compatibility - Proto field numbers: field 9 is kept (deprecated, not removed). Field 10 is new. No field number reuse. - Old readers: ignore unknown field 10; they only read field 9, which is now empty on new commits. Old Lance versions deserializing commits written by this PR will not recover offsets from the txn; that only affects audit/`readTransaction()` on historical commits, not table data or OCC. - New readers: prefer field 10; fall back to field 9 for manifests written by older Lance versions that predate this change. - No JNI or Java changes. The in-memory type (`UpdatedFragmentOffsets`) is unchanged; only the proto wire encoding changes. Independent of #6748 and lance-spark #528 (JNI wiring). No mutual merge dependencies. Co-authored-by: Jing chen He <jingh@adobe.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
2 participants
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.
Regular table scan go in order. KNN scan can be unordered