Skip to content

fix: set maximan 8 target partitions for merge insert update fragments#3603

Merged
LuQQiu merged 2 commits intolance-format:mainfrom
LuQQiu:improveMergeInsert
Mar 26, 2025
Merged

fix: set maximan 8 target partitions for merge insert update fragments#3603
LuQQiu merged 2 commits intolance-format:mainfrom
LuQQiu:improveMergeInsert

Conversation

@LuQQiu
Copy link
Copy Markdown
Contributor

@LuQQiu LuQQiu commented Mar 26, 2025

fixes #3601
More info can be find in the #3601
For merge_insert, the partition number does not affect the memory size required by each partition but affect the memory size that is available for this partition.
Limit the target partitions to 8 or CPU cores to reduce the chance of hitting Resources exhausted during merge insert

@LuQQiu LuQQiu requested review from westonpace and wjones127 March 26, 2025 00:48
@github-actions github-actions Bot added the bug Something isn't working label Mar 26, 2025
@LuQQiu LuQQiu requested a review from wkalt March 26, 2025 00:48
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.69%. Comparing base (babb5ab) to head (16a1a65).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3603      +/-   ##
==========================================
+ Coverage   78.65%   78.69%   +0.03%     
==========================================
  Files         258      258              
  Lines       96782    96897     +115     
  Branches    96782    96897     +115     
==========================================
+ Hits        76126    76255     +129     
+ Misses      17588    17571      -17     
- Partials     3068     3071       +3     
Flag Coverage Δ
unittests 78.69% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread rust/lance-datafusion/src/exec.rs Outdated
if options.target_partition.is_some() {
return new_session_context(options);
}
if options.mem_pool_size() == DEFAULT_LANCE_MEM_POOL_SIZE {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: the changes to this method could be simplified if you changed this line to:

if options.mem_pool_size() == DEFAULT_LANCE_MEM_POOL_SIZE && options.target_partition.is_none() {

Copy link
Copy Markdown
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks for looking into this

@LuQQiu LuQQiu merged commit cd44f55 into lance-format:main Mar 26, 2025
27 checks passed
Jay-ju pushed a commit to Jay-ju/lance that referenced this pull request Mar 28, 2025
lance-format#3603)

fixes lance-format#3601 
More info can be find in the lance-format#3601 
For merge_insert, the partition number does not affect the memory size
required by each partition but affect the memory size that is available
for this partition.
Limit the target partitions to 8 or CPU cores to reduce the chance of
hitting Resources exhausted during merge insert
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

merge_insert spill configuration easily too small if server has many cores

3 participants