-
Notifications
You must be signed in to change notification settings - Fork 414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: promql engine to load data in steps #3783
Conversation
WalkthroughThe updates primarily involve refinements to data processing logic in a Rust project using DataFusion, including enhanced data aggregation, filtering, and selection mechanisms for metrics retrieval. Additionally, there are adaptations in how histogram bucket labels are handled and a renaming of constants for improved readability. 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/service/promql/engine.rs (4 hunks)
Additional context used
Path-based instructions (1)
src/service/promql/engine.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 (3)
src/service/promql/engine.rs (3)
25-25
: Ensure that the newly introducedmax
function is utilized appropriately in the context of aggregations or wherever maximum values are needed.
1134-1188
: The logic for filtering and aggregating data appears to be correct. However, consider optimizing the data retrieval and processing by reducing the number of clones and potentially caching frequently accessed data.
[REFACTOR_SUGGESTion]// Possible optimization by reducing clones and using more efficient data structures or caching mechanisms.
Line range hint
1219-1252
: This batch processing section is critical for performance. Ensure that the sorting and filtering operations are optimized for large datasets. Consider using more efficient data structures or algorithms if performance issues arise.
0870f42
to
1f87ce4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
src/service/promql/engine.rs (1)
47-48
: Clarify the purpose ofcol_filters
.Consider adding a comment explaining the role of
col_filters
in the context of the Engine struct, especially since it plays a crucial role in the new data loading strategy.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/service/promql/engine.rs (9 hunks)
Additional context used
Path-based instructions (1)
src/service/promql/engine.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 (5)
src/service/promql/engine.rs (5)
25-25
: Refactor import statement for clarity.The import line includes
max
which is a new addition. Make sure it's used appropriately in the context of the file.
92-104
: Optimize the handling of column filters.The logic for extending
col_filters
based onVectorMatchCardinality
andBinModifier
is clear and seems efficient. However, ensure that the performance implications of dynamically modifyingcol_filters
during query parsing are well-understood and documented.
1069-1082
: Optimize label column handling.The extraction of label columns could potentially be optimized. Consider caching commonly used label columns or reusing them across different parts of the code to avoid repeated schema lookups.
[REFACTOR_Suggestion]- let label_cols = df_group - .schema() - .fields() - .iter() - .filter_map(|field| { - let name = field.name(); - if name == &cfg.common.column_timestamp || name == VALUE_LABEL { - None - } else { - Some(col(name)) - } - }) - .collect::<Vec<_>>(); + // Example refactor to cache label columns + static LABEL_COLS: OnceCell<Vec<_>> = OnceCell::new(); + let label_cols = LABEL_COLS.get_or_init(|| { + df_group + .schema() + .fields() + .iter() + .filter_map(|field| { + let name = field.name(); + if name != &cfg.common.column_timestamp && name != VALUE_LABEL { + Some(col(name)) + } else { + None + } + }) + .collect::<Vec<_>>() + });
138-144
: Review the clearing ofcol_filters
.Clearing
col_filters
in certain cases (like topk and bottomk queries) could lead to unintended side effects if not handled carefully. Verify that this behavior is intended and test for edge cases where this might lead to incorrect data filtering.Verification successful
Clearing of
col_filters
is correctly handled.The
eval_top
function, which is used by bothtopk
andbottomk
, processes the clearedcol_filters
by applying the appropriate label modifiers (Include and Exclude). This ensures that the intended behavior is maintained and no unintended side effects occur.
src/service/promql/engine.rs
: Lines 138-144src/service/promql/aggregations/mod.rs
:eval_top
functionScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify edge cases for clearing col_filters in topk and bottomk queries. # Test: Search for usages of topk and bottomk to ensure they handle the clear operation correctly. rg --type rust $'token::T_TOPK | token::T_BOTTOMK'Length of output: 257
Script:
#!/bin/bash # Extract lines around the identified usages of token::T_TOPK and token::T_BOTTOMK to understand the context rg --type rust -A 10 -B 10 'token::T_TOPK | token::T_BOTTOMK' src/service/promql/engine.rsLength of output: 2109
Script:
#!/bin/bash # Search for the implementations of aggregations::topk and aggregations::bottomk rg --type rust 'fn topk' --context 10 rg --type rust 'fn bottomk' --context 10Length of output: 2702
Script:
#!/bin/bash # Search for the implementation of eval_top to understand how it handles the cleared col_filters rg --type rust 'fn eval_top' --context 10Length of output: 1348
Script:
#!/bin/bash # Extract the complete implementation of eval_top function to understand its handling of col_filters rg --type rust -A 50 'fn eval_top' src/service/promql/aggregations/mod.rsLength of output: 1886
1040-1058
: Ensure selected columns are necessary.The selection of columns based on
col_filters
is a critical operation. Double-check that all necessary columns (HASH_LABEL
,cfg.common.column_timestamp
,VALUE_LABEL
) are indeed required and used efficiently in subsequent operations.Verification successful
Selected columns are necessary and used.
The columns
HASH_LABEL
,cfg.common.column_timestamp
, andVALUE_LABEL
are indeed used extensively throughout the codebase. The search results confirm their necessity in various operations across multiple modules.
HASH_LABEL
is used insrc/service/promql/engine.rs
,src/service/metrics/mod.rs
, and other files.cfg.common.column_timestamp
is used insrc/service/promql/engine.rs
,src/service/schema.rs
, and numerous other files.VALUE_LABEL
is used insrc/service/promql/engine.rs
,src/service/metrics/prom.rs
, and several other files.These usages indicate that the selected columns are integral to the functioning of the system.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that selected columns are used in subsequent operations. # Test: Search for usages of the selected columns to ensure they are necessary. rg --type rust $'HASH_LABEL|cfg.common.column_timestamp|VALUE_LABEL'Length of output: 14348
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/service/promql/engine.rs (9 hunks)
Additional context used
Path-based instructions (1)
src/service/promql/engine.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 (11)
src/service/promql/engine.rs (11)
25-25
: Import optimally utilized.The addition of
max
function from DataFusion's prelude aligns with the changes in the aggregation methods in this file.
47-48
: Clarification on column filters.The comment on line 47 has been simplified to directly reflect the purpose of
col_filters
. This change improves code readability and understanding.
57-57
: Initialization ofcol_filters
.Initializing
col_filters
with an emptyHashSet
is appropriate here, ensuring that no columns are filtered unless specified later in the code.
93-110
: Refinement in column extraction logic.The logic for extracting columns based on binary expression modifiers has been refined. The handling of
group_left
with labels is particularly noteworthy, ensuring only specified labels are selected. This is crucial for performance and correctness in data querying.
144-150
: Simplification of column filtering logic.The clearing of
col_filters
for specific operations liketopk
andbottomk
ensures that all columns are considered during these operations, which is necessary for their correct execution. This change simplifies and clarifies the behavior.
1046-1065
: Optimized column selection.The logic for including only necessary columns (
__hash__
,_timestamp
,value
) and any additional specified labels is a critical optimization. This prevents unnecessary data loading and processing, aligning with the PR's goal of improving performance.
1075-1087
: Efficient label column handling.The method for filtering out timestamp and value columns when collecting label columns is efficient and prevents redundancy. This contributes to the streamlined processing of data.
1089-1097
: Aggregation logic for timestamp.The use of the
max
function to aggregate timestamps is a smart choice, ensuring that only the most recent timestamps are processed further. This is an effective use of DataFusion's capabilities.
1119-1125
: Filter application in data selection.Applying a filter based on the most recent timestamps to fetch relevant series data is a crucial step that ensures efficiency in data handling and aligns with the overall optimization goals of the PR.
1135-1157
: Hash-based data filtering and label management.The logic to filter data based on hash values and to manage labels efficiently ensures that only relevant data is processed. This is essential for performance and memory usage optimization.
1185-1190
: Efficient sample appending.Appending samples based on hash values directly into the corresponding
RangeValue
is an efficient way to handle incoming data streams. This method minimizes overhead and aligns with the performance objectives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/service/promql/engine.rs (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/service/promql/engine.rs
da0457e
to
925a54a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/service/promql/engine.rs (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/service/promql/engine.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/service/promql/engine.rs (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/service/promql/engine.rs
c31abcc
to
925a54a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/service/promql/engine.rs (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/service/promql/engine.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/common/meta/prom.rs (1 hunks)
- src/service/promql/engine.rs (8 hunks)
- src/service/promql/functions/histogram.rs (2 hunks)
Files skipped from review due to trivial changes (1)
- src/common/meta/prom.rs
Files skipped from review as they are similar to previous changes (1)
- src/service/promql/engine.rs
Additional context used
Path-based instructions (1)
src/service/promql/functions/histogram.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/service/promql/functions/histogram.rs (2)
20-20
: Update to constants is consistent with project-wide changes.The renaming of
LE_LABEL
toBUCKET_LABEL
across the project is reflected here, ensuring consistency.
70-70
: Proper error handling for bucket label parsing.The use of
match
for error handling when parsing theBUCKET_LABEL
value is idiomatic in Rust and ensures that the loop continues smoothly in case of parsing errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/service/promql/engine.rs (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/service/promql/engine.rs
fixed #2710 |
This pr breaks the data loading process into two steps by utilizing `__hash__` columns. 1. find the unique combinations of `__hash__` and all k-v pairs of labels 2. load only `__hash__`, `_timestamp`, and `value` columns into memory for querying --------- Co-authored-by: Hengfei Yang <hengfei.yang@gmail.com>
While previous pr #3736 improved both memory/throughput for aggregation, binary, and unary PromQL queries by loading only necessary columns into memory. Other queries still load the entire parquet file, resulting in high memory usage and slow throughput.
This pr breaks the data loading process into two steps by utilizing
__hash__
columns.__hash__
and all k-v pairs of labels__hash__
,_timestamp
, andvalue
columns into memory for queryingThis will greatly improve the performance for vector/matrix selector queries.
Simple metric selector query, e.g.
container_memory_usage_bytes
Before
After
Aggregation query, e.g.
max by (node) (container_memory_usage_bytes)
raised by #3091Before
After
PromQL compliance result
Summary by CodeRabbit
New Features
Refactor
LE_LABEL
toBUCKET_LABEL
to improve clarity in histogram processing.Improvements