feat: multi search with multiple time ranges #4626
Conversation
WalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 2
Outside diff range and nitpick comments (3)
src/config/src/meta/search.rs (3)
742-753: Consider makingis_old_formatfield privateThe
is_old_formatfield is used internally for deserialization and isn't required to be publicly accessible. Making it private would enhance encapsulation.
742-753: Add documentation toSqlQuerystruct and its fieldsAdding doc comments to the
SqlQuerystruct and its fields would improve code readability and help others understand its purpose.
791-792: Add documentation forper_query_responsefieldIncluding a doc comment for the
per_query_responsefield would clarify its purpose and usage.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/config/src/meta/search.rs (3 hunks)
- src/handler/http/request/search/multi_streams.rs (9 hunks)
Additional context used
Path-based instructions (2)
src/config/src/meta/search.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/handler/http/request/search/multi_streams.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 (8)
src/handler/http/request/search/multi_streams.rs (5)
134-136: Improved error handling with block statementsThe error handling has been refactored to use block statements, which enhances readability and maintainability. This change is consistent across multiple error checks in the function.
Also applies to: 141-143, 149-151
169-170: New feature: Per-query response aggregationA new flag
per_query_resphas been introduced to control how hits are aggregated in the multi-query response. This provides more flexibility in the API, allowing clients to receive either a flat list of hits or separate arrays of hits for each query.Also applies to: 350-354
476-478: Consistent error handling improvementThe error handling in
_search_partition_multihas been updated to use block statements, consistent with the changes insearch_multi. This improves code readability and maintainability across the file.Also applies to: 483-485
622-624: Consistent error handling in around_multiThe error handling in
around_multihas been updated to use block statements, maintaining consistency with the other functions in this file. This change improves the overall readability and maintainability of the codebase.Also applies to: 629-631
Line range hint
1-924: Overall improvements in error handling and response flexibilityThe changes in this file demonstrate a consistent approach to improving error handling across all functions. The use of block statements for error handling enhances readability and maintainability. Additionally, the introduction of the
per_query_respflag in thesearch_multifunction provides more flexibility in response aggregation, allowing clients to receive either a flat list of hits or separate arrays of hits for each query.These changes contribute to a more robust and flexible implementation of multi-stream search operations. The consistency in applying these improvements across all functions is commendable and will likely make future maintenance easier.
src/config/src/meta/search.rs (3)
19-19: LGTM!The import of
Deserializeris appropriate for the custom deserialization function.
758-759: LGTM!The custom deserialization for the
sqlfield usingdeserialize_sqlis correctly implemented.
837-847: LGTM!The
Querystruct is correctly constructed with appropriate handling ofsql,start_time,end_time, andquery_fn.
| fn deserialize_sql<'de, D>(deserializer: D) -> Result<Vec<SqlQuery>, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| #[derive(Deserialize)] | ||
| #[serde(untagged)] | ||
| enum SqlOrSqlQuery { | ||
| OldFormat(String), | ||
| NewFormat(SqlQuery), | ||
| } | ||
|
|
||
| let v: Vec<SqlOrSqlQuery> = Vec::deserialize(deserializer)?; | ||
|
|
||
| // Convert old format into the new format | ||
| let result: Vec<SqlQuery> = v | ||
| .into_iter() | ||
| .map(|item| match item { | ||
| SqlOrSqlQuery::OldFormat(sql) => SqlQuery { | ||
| sql, | ||
| start_time: None, | ||
| end_time: None, | ||
| query_fn: None, | ||
| is_old_format: true, | ||
| }, | ||
| SqlOrSqlQuery::NewFormat(query) => query, | ||
| }) | ||
| .collect(); | ||
|
|
||
| Ok(result) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests for deserialize_sql function
Introducing unit tests for the deserialize_sql function will ensure both old and new SQL formats are correctly handled and prevent future regressions.
| let query_fn = if query.is_old_format { | ||
| self.query_fn.clone() | ||
| } else { | ||
| query.query_fn.clone() | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encapsulate is_old_format usage within SqlQuery
Accessing is_old_format outside of SqlQuery can break encapsulation. Consider adding a method within SqlQuery to provide the appropriate query_fn, or handle this logic internally.
oasisk
force-pushed
the
feat-multi-search-with-multiple-time-ranges
branch
from
124ea07 to
4931416
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
src/config/src/meta/search.rs (3)
758-759: LGTM: EnhancedMultiStreamRequeststructThe changes to the
MultiStreamRequeststruct improve its functionality:
- The
sqlfield now usesVec<SqlQuery>, allowing for more detailed query information.- The new
per_query_responsefield has been added, which likely controls how responses are structured.- The custom deserialization function for
sqlallows for backward compatibility.These changes enhance the flexibility of the struct while maintaining compatibility with older formats.
Consider adding a comment to explain the purpose and usage of the
per_query_responsefield for better documentation.Also applies to: 791-793
795-824: LGTM: Well-implementeddeserialize_sqlfunctionThe
deserialize_sqlfunction is well-implemented to handle both old and new formats of SQL queries. It uses an untagged enum approach, which is an elegant solution for this scenario. The conversion logic ensures backward compatibility by setting theis_old_formatflag for old format queries.Consider using
Vec::with_capacity(v.len())when creating theresultvector to avoid potential reallocations:- let result: Vec<SqlQuery> = v + let mut result: Vec<SqlQuery> = Vec::with_capacity(v.len()); + result.extend(v .into_iter() .map(|item| match item { SqlOrSqlQuery::OldFormat(sql) => SqlQuery { sql, start_time: None, end_time: None, query_fn: None, is_old_format: true, }, SqlOrSqlQuery::NewFormat(query) => query, - }) - .collect(); + }));This small optimization can improve performance, especially for large input vectors.
829-848: LGTM: Updatedto_query_reqmethod handles newSqlQuerystructure wellThe changes to the
to_query_reqmethod correctly handle the newSqlQuerystructure while maintaining backward compatibility. The use of theis_old_formatflag to determine whichquery_fnto use is a good approach. The method also properly handles the optionalstart_timeandend_timevalues from theSqlQuery.Consider extracting the
query_fnlogic into a separate method for improved readability:impl MultiStreamRequest { fn get_query_fn(&self, query: &SqlQuery) -> Option<String> { if query.is_old_format { self.query_fn.clone() } else { query.query_fn.clone() } } pub fn to_query_req(&mut self) -> Vec<Request> { let mut res = vec![]; for query in &self.sql { let query_fn = self.get_query_fn(query); res.push(Request { query: Query { sql: query.sql.clone(), // ... rest of the fields ... query_fn, // ... rest of the fields ... }, // ... rest of the fields ... }); } res } }This change would make the
to_query_reqmethod more concise and easier to understand.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/config/src/meta/search.rs (3 hunks)
- src/handler/http/request/search/multi_streams.rs (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/handler/http/request/search/multi_streams.rs
Additional context used
Path-based instructions (1)
src/config/src/meta/search.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 (2)
src/config/src/meta/search.rs (2)
742-753: LGTM: Well-structuredSqlQuerystructThe new
SqlQuerystruct is well-designed to encapsulate SQL query information. The use ofOption<i64>forstart_timeandend_timeallows for nullable values, and theis_old_formatflag is useful for backward compatibility.
Line range hint
1-1161: Overall assessment: Well-implemented multi-search with multiple time rangesThe changes introduced in this file successfully implement multi-search functionality with support for multiple time ranges. Key improvements include:
- Introduction of the
SqlQuerystruct to encapsulate query information.- Updates to the
MultiStreamRequeststruct to use the newSqlQuerytype.- Implementation of a custom deserialization function to handle both old and new query formats.
- Updates to the
to_query_reqmethod to work with the new structures.These changes enhance the flexibility and functionality of the search system while maintaining backward compatibility. The code is well-structured and the new features integrate seamlessly with the existing codebase.
oasisk
force-pushed
the
feat-multi-search-with-multiple-time-ranges
branch
from
4931416 to
d8dbcc4
Compare
oasisk
force-pushed
the
feat-multi-search-with-multiple-time-ranges
branch
from
02405d6 to
e40b1b4
Compare
Summary by CodeRabbit
New Features
SqlQuerystructure for improved SQL query handling.per_query_responsefeature to enhance query response flexibility.Bug Fixes
Refactor
search_multiand_search_partition_multifunctions for improved code clarity.