fix: target partition for noCache #4484
Conversation
WalkthroughThe changes involve modifications to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Outside diff range, codebase verification and nitpick comments (1)
src/service/search/grpc/mod.rs (1)
Line range hint
122-161: Approved: Updated logic insearchfunction.The updates to handle the new
target_partitionslogic are correctly implemented. Consider adding tests to verify thattarget_partitionsis correctly updated based on the search results.Would you like me to help in writing these tests?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/service/search/datafusion/exec.rs (1 hunks)
- src/service/search/grpc/mod.rs (3 hunks)
- src/service/search/grpc/storage.rs (3 hunks)
- src/service/search/grpc/wal.rs (8 hunks)
Additional context used
Path-based instructions (4)
src/service/search/grpc/mod.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/grpc/wal.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/grpc/storage.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/search/datafusion/exec.rs (1)
Pattern
**/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestionsMake sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
Additional comments not posted (4)
src/service/search/grpc/mod.rs (1)
47-47: Approved: UpdatedSearchTabletype definition.The addition of
usizeto theSearchTabletype is consistent with the PR objectives. Ensure that this new type is used consistently throughout the file.Verification successful
Verified: Consistent usage of updated
SearchTabletype.The
SearchTabletype, updated to includeusize, is used consistently across the relevant files. No issues found with its implementation or usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new `SearchTable` type throughout the file. # Test: Search for the usage of `SearchTable`. Expect: Only occurrences of the new type. rg --type rust -A 5 $'SearchTable'Length of output: 1846
src/service/search/grpc/wal.rs (2)
Line range hint
58-280: Approved changes with a suggestion to verify integration.The function
search_parquetnow returns asuper::SearchTableinstead of a tuple, aligning with the new requirements to provide additional context through an integer value. The changes are consistent and correctly implemented across different return points.Please ensure that the integration of the new return type
super::SearchTableis compatible with the rest of the system, particularly where this function is called.
Line range hint
280-376: Approved changes with a suggestion to verify integration.The function
search_memtablenow returns asuper::SearchTableinstead of a tuple, aligning with the new requirements to provide additional context through an integer value. The changes are consistent and correctly implemented across different return points.Please ensure that the integration of the new return type
super::SearchTableis compatible with the rest of the system, particularly where this function is called.src/service/search/grpc/storage.rs (1)
Line range hint
88-280: Approved changes with a suggestion to verify integration.The function
searchnow returns a tuple includingtarget_partitionsas an additional integer value, aligning with the new requirements to provide more detailed results. The changes are consistent and correctly implemented across different return points.Please ensure that the integration of the new return type is compatible with the rest of the system, particularly where this function is called.
| // register empty table | ||
| let empty_table = NewEmptyTable::new("tbl", schema.clone(), sort_by_timestamp_desc) | ||
| .with_partitions(cfg.limit.cpu_num); | ||
| .with_partitions(session.target_partitions); |
There was a problem hiding this comment.
Adjustment to partition management in sql function.
The change from using cfg.limit.cpu_num to session.target_partitions for the with_partitions method in the NewEmptyTable instance is a significant shift. This adjustment allows for dynamic partitioning based on session-specific settings rather than a static configuration tied to CPU limits. This should provide more flexibility in handling different workloads and session requirements.
Ensure that the session.target_partitions is properly validated and falls within acceptable limits to prevent potential runtime issues due to misconfiguration.
Consider adding validation for session.target_partitions to ensure it's within a reasonable range before applying it as a partition parameter.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation