perf: improve performance of CandidateBin.split_for_size - #8000
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughRewrites ChangesCandidate bin splitting
Estimated code review effort: 3 (Moderate) | ~15–30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/optimize.rs`:
- Around line 2371-2393: Update the Fragment literal inside tail_too_small_bin
to initialize the required overlays field with an empty vector, matching
Fragment::new(), while leaving the split_for_size assertions unchanged.
🪄 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: QUIET
Plan: Pro Plus
Run ID: 612bb216-91e4-43e7-a4f9-437f1d3f0e9e
📒 Files selected for processing (1)
rust/lance/src/dataset/optimize.rs
|
|
||
| let tail_too_small_bin = CandidateBin { | ||
| fragments: std::iter::repeat_n( | ||
| Fragment { | ||
| id: 0, | ||
| files: vec![], | ||
| deletion_file: None, | ||
| row_id_meta: None, | ||
| physical_rows: Some(0), | ||
| last_updated_at_version_meta: None, | ||
| created_at_version_meta: None, | ||
| }, | ||
| 3, | ||
| ) | ||
| .collect(), | ||
| pos_range: 0..3, | ||
| candidacy: std::iter::repeat_n(CompactionCandidacy::CompactItself, 3).collect(), | ||
| row_counts: vec![600, 100, 100], | ||
| indices: vec![], | ||
| }; | ||
| let split = tail_too_small_bin.split_for_size(500); | ||
| assert_eq!(split.len(), 1); | ||
| assert_eq!(split[0].pos_range, 0..3); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the required fields of lance_table::format::Fragment
rg -nP -A 25 '^pub struct Fragment\b' rust/lance-table/src/formatRepository: lance-format/lance
Length of output: 2246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate optimize.rs =="
fd -a 'optimize\.rs$' rust | sed 's#^\./##'
echo "== count Fragment literals in tail_too_small range =="
sed -n '2360,2396p' rust/lance/src/dataset/optimize.rs | nl -ba -v2360
echo "== Fragment struct full field list =="
python3 - <<'PY'
from pathlib import Path
p=Path('rust/lance-table/src/format/fragment.rs')
txt=p.read_text()
start=txt.index('pub struct Fragment {')
end=txt.index('}', start)+1
struct=txt[start:end]
print(struct)
print("fields:", [line.strip().removesuffix(',').split(':',1)[0].strip() for line in struct.splitlines() if ':' in line and not line.strip().startswith('///') and not line.strip().startswith('#[')])
PY
echo "== check whether FileFragment has overlays field =="
rg -nP -A 12 '^pub struct FileFragment\b|pub overlays' rust/lance-table/src/format/fragment.rsRepository: lance-format/lance
Length of output: 452
🏁 Script executed:
#!/bin/sh
set -euo pipefail
echo "== optimize.rs tail_too_small block =="
sed -n '2360,2396p' rust/lance/src/dataset/optimize.rs | cat -n
echo "== Fragment struct definition and FileFragment overlay field =="
python3 - <<'PY'
from pathlib import Path
p = Path('rust/lance-table/src/format/fragment.rs')
text = p.read_text()
start_struct = text.index('pub struct Fragment {')
# find matching brace is optional for next extraction; stop at next matching brace roughly by line indentation
lines = text.splitlines()
found = False
field_lines = []
brace_nesting = 0
for i, line in enumerate(lines, 1):
if found:
field_lines.append(f"{i}: {line}")
brace_nesting += line.count('{') - line.count('}')
if brace_nesting == 0 and '}' in line:
break
if line.strip() == 'pub struct Fragment {':
found = True
print("\nFragment struct:")
print("\n".join(field_lines))
ff_start = text.index('pub struct FileFragment {')
ff_lines = []
brace_nesting = 0
found = False
for i, line in enumerate(lines, 1):
if found:
ff_lines.append(f"{i}: {line}")
brace_nesting += line.count('{') - line.count('}')
if brace_nesting == 0 and '}' in line:
break
if line.strip() == 'pub struct FileFragment {':
found = True
print("\nFileFragment struct:")
print("\n".join(ff_lines))
PYRepository: lance-format/lance
Length of output: 3828
Don’t add a Fragment literal without overlays.
This Fragment struct literal still omits the required pub overlays: Vec<DataOverlayFile> field, so the test will fail to compile with E0063. Add overlays: vec![] here, matching how Fragment::new() initializes that field.
🧰 Tools
🪛 GitHub Check: format
[warning] 2382-2382:
Diff in /home/runner/work/lance/lance/rust/lance/src/dataset/optimize.rs
🤖 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/optimize.rs` around lines 2371 - 2393, Update the
Fragment literal inside tail_too_small_bin to initialize the required overlays
field with an empty vector, matching Fragment::new(), while leaving the
split_for_size assertions unchanged.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rust/lance/src/dataset/optimize.rs (1)
2372-2393: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winMissing required
overlaysfield onFragmentliteral — still breaks compilation.This
Fragmentliteral omitsoverlays: Vec<DataOverlayFile>, unlike the otherFragmentliteral earlier in the same test (line 2330) which correctly setsoverlays: vec![]. This will fail to compile with E0063 (missing field).🐛 Proposed fix
fragments: std::iter::repeat_n( Fragment { id: 0, files: vec![], + overlays: vec![], deletion_file: None, row_id_meta: None, physical_rows: Some(0), last_updated_at_version_meta: None, created_at_version_meta: None, }, 3, ) .collect(),🤖 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/optimize.rs` around lines 2372 - 2393, Update the Fragment literal in the tail_too_small_bin test fixture to initialize the required overlays field with an empty vector, matching the other Fragment literal in the same test. Do not change the split_for_size assertions or other fixture behavior.
🤖 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.
Duplicate comments:
In `@rust/lance/src/dataset/optimize.rs`:
- Around line 2372-2393: Update the Fragment literal in the tail_too_small_bin
test fixture to initialize the required overlays field with an empty vector,
matching the other Fragment literal in the same test. Do not change the
split_for_size assertions or other fixture behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: b6137db2-1a75-4dcd-94b7-8a113d763f28
📒 Files selected for processing (1)
rust/lance/src/dataset/optimize.rs
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@yanghua @zhangyue19921010 Could you please review this? |
|
|
||
| // Only split once the current bin is large enough and there is | ||
| // enough left over to form another worthwhile bin. | ||
| if current_rows >= min_num_rows && remaining_rows >= min_num_rows { |
There was a problem hiding this comment.
P2: When target_rows_per_fragment is 0, split_for_size creates an empty trailing bin, which eventually causes a panic when commit logic accesses old_fragments[0]. Reject zero at the API boundary or avoid creating the final empty bin, and add a regression test.
There was a problem hiding this comment.
Great catch ! In the context of Compaction, target_rows_per_fragments is allowed to be 0. Therefore, I fixed an issue in the split_for_size method where an empty CandidateBin could be produced when min_num_rows = 0. In addition, I added test cases for min_num_rows = 0.
db8bf38 to
e62e996
Compare
zhangyue19921010
left a comment
zhangyue19921010
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the precise fix! @yanghua Would u mind to give a final pass?
2 participants
Close #7967