fix: don't fail dataset open on an undecodable inline transaction#7740
Open
jackye1995 wants to merge 1 commit into
Open
fix: don't fail dataset open on an undecodable inline transaction#7740jackye1995 wants to merge 1 commit into
jackye1995 wants to merge 1 commit into
Conversation
Opening a dataset eagerly decodes the inline manifest transaction section to warm the session cache. A transaction written by a newer version of Lance can contain an operation type this version cannot decode, which made the whole dataset version unopenable even though the transaction contents are not needed to read data. Tolerate the decode failure (log and skip caching); paths that actually need the transaction contents still surface the error at their call sites.
📝 WalkthroughWalkthroughChangesInline transaction tolerance
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance/src/dataset.rs`:
- Around line 3673-3677: Update Transaction::try_from in the Update and
UpdateMemWalState conversion paths to propagate MergedGeneration::try_from
failures instead of unwrapping, allowing decode_inline_transaction to return
None for malformed inline updates. Add a regression test covering corrupt inline
update data during dataset opening.
In `@rust/lance/src/dataset/tests/dataset_transactions.rs`:
- Around line 213-215: Move the `decode_inline_transaction`,
`lance_table::format::pb`, and `prost::Message` imports from the test function
to module scope at the top of `dataset_transactions.rs`, leaving their usage
unchanged.
- Around line 220-225: Update the transaction test around unknown_operation so
it encodes a non-empty, unrecognized operation field, ensuring this assertion
exercises the unknown-operation path. Preserve the existing fixture separately
for the missing-operation case, where operation remains unset, and keep the
expected decode_inline_transaction result as None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a8826953-c333-4e16-bce2-7799b1b1216a
📒 Files selected for processing (2)
rust/lance/src/dataset.rsrust/lance/src/dataset/tests/dataset_transactions.rs
Comment on lines
+3673
to
+3677
| fn decode_inline_transaction(message_data: &[u8], version: u64) -> Option<Transaction> { | ||
| match lance_table::format::pb::Transaction::decode(message_data) | ||
| .map_err(Error::from) | ||
| .and_then(Transaction::try_from) | ||
| { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant areas around decode_inline_transaction and Transaction::try_from
sed -n '3650,3705p' rust/lance/src/dataset.rs
printf '\n====\n'
sed -n '1,260p' rust/lance/src/dataset/transaction.rs
printf '\n====\n'
rg -n "MergedGeneration::try_from\\(|unwrap\\(|Transaction::try_from|decode_inline_transaction" rust/lance/src/dataset/transaction.rs rust/lance/src/dataset.rsRepository: lance-format/lance
Length of output: 23925
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any tests or call sites that rely on decode_inline_transaction returning None on malformed data.
rg -n "decode_inline_transaction|inline transaction|Transaction::decode|MergedGeneration::try_from" rust/lance -g '!target'Repository: lance-format/lance
Length of output: 2198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Transaction::try_from branches that unwrap MergedGeneration::try_from
sed -n '3160,3365p' rust/lance/src/dataset/transaction.rs
printf '\n====\n'
sed -n '6180,6245p' rust/lance/src/dataset/transaction.rs
printf '\n====\n'
sed -n '200,250p' rust/lance/src/dataset/tests/dataset_transactions.rsRepository: lance-format/lance
Length of output: 12618
Propagate malformed inline Update conversions instead of panicking.
Transaction::try_from still unwraps MergedGeneration::try_from(m) in the Update and UpdateMemWalState paths, so a corrupt inline update can abort dataset opening instead of falling back to None. Return that conversion error and add a regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance/src/dataset.rs` around lines 3673 - 3677, Update
Transaction::try_from in the Update and UpdateMemWalState conversion paths to
propagate MergedGeneration::try_from failures instead of unwrapping, allowing
decode_inline_transaction to return None for malformed inline updates. Add a
regression test covering corrupt inline update data during dataset opening.
Source: Coding guidelines
Comment on lines
+213
to
+215
| use crate::dataset::decode_inline_transaction; | ||
| use lance_table::format::pb; | ||
| use prost::Message; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move these imports to module scope.
The test-local use declarations should be placed at the top of rust/lance/src/dataset/tests/dataset_transactions.rs.
As per coding guidelines: Rust use imports must be placed at the top of the file, not inside function bodies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance/src/dataset/tests/dataset_transactions.rs` around lines 213 - 215,
Move the `decode_inline_transaction`, `lance_table::format::pb`, and
`prost::Message` imports from the test function to module scope at the top of
`dataset_transactions.rs`, leaving their usage unchanged.
Source: Coding guidelines
Comment on lines
+220
to
+225
| let unknown_operation = pb::Transaction { | ||
| read_version: 1, | ||
| uuid: "test".to_string(), | ||
| ..Default::default() | ||
| }; | ||
| assert!(decode_inline_transaction(&unknown_operation.encode_to_vec(), 42).is_none()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test and decoder code
git ls-files | rg 'rust/lance/src/dataset/tests/dataset_transactions.rs|decode_inline_transaction|transaction'
rg -n "decode_inline_transaction|unknown_operation|operation: None|Transaction" rust/lance/src/dataset/tests/dataset_transactions.rs rust/lance/src -g '!**/target/**'
# Show the surrounding test lines
sed -n '180,260p' rust/lance/src/dataset/tests/dataset_transactions.rs
# Find the protobuf definition for Transaction
rg -n "message Transaction|oneof operation|operation =" rust -g '*.proto' -g '*.rs'Repository: lance-format/lance
Length of output: 48965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect decoder implementation and protobuf-generated accessors around the transaction type
rg -n "fn decode_inline_transaction|decode_inline_transaction" rust/lance/src -g '*.rs'
rg -n "pub struct Transaction|impl Transaction|enum Operation|oneof" rust -g '*.rs' -g '*.proto'
# If the generated Rust file is available, inspect the relevant section
python3 - <<'PY'
from pathlib import Path
paths = list(Path('rust').rglob('*.rs'))
for p in paths:
txt = p.read_text(errors='ignore')
if 'decode_inline_transaction' in txt or 'struct Transaction' in txt and 'uuid' in txt and 'read_version' in txt:
print(p)
PYRepository: lance-format/lance
Length of output: 2822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the decoder and protobuf conversion paths around the relevant match arms
sed -n '3060,3370p' rust/lance/src/dataset/transaction.rs
printf '\n---\n'
sed -n '3438,3478p' rust/lance/src/dataset/transaction.rs
printf '\n---\n'
sed -n '3668,3692p' rust/lance/src/dataset.rs
# Inspect the protobuf schema for Transaction
sed -n '1,120p' protos/transaction.protoRepository: lance-format/lance
Length of output: 20014
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any existing test that intentionally appends unknown protobuf fields
rg -n "unknown field|unrecognized|append.*field|varint|tag" rust/lance/src rust/lance-table/src protos -g '*.rs' -g '*.proto'Repository: lance-format/lance
Length of output: 50375
Split the missing-operation and unknown-operation cases. This fixture only leaves operation unset, so it exercises the missing-operation path. Keep that case separate and use bytes with an unrecognized operation field here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance/src/dataset/tests/dataset_transactions.rs` around lines 220 - 225,
Update the transaction test around unknown_operation so it encodes a non-empty,
unrecognized operation field, ensuring this assertion exercises the
unknown-operation path. Preserve the existing fixture separately for the
missing-operation case, where operation remains unset, and keep the expected
decode_inline_transaction result as None.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
1 participant
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.
Opening a dataset eagerly decodes the inline manifest transaction section (added in v1.0) to warm the session cache. If the transaction was written by a newer version of Lance with an operation type this version cannot decode, the decode error fails the whole
load_manifestcall — making the dataset version unopenable even though the transaction contents are not needed to read data. This already applies to recently added operation types (e.g.UpdateBases,Clone,UpdateMemWalState) read by older 1.x releases, and would apply to any operation added in the future.Since this decode is purely an opportunistic cache warm-up, tolerate the failure: log a warning and skip caching. Paths that actually need the transaction contents (
read_transaction, conflict resolution) still read and surface errors at their call sites.Summary by CodeRabbit
Bug Fixes
Tests