chore: avoid some unnecessary hash lookups#7576
chore: avoid some unnecessary hash lookups#7576fengjiachun wants to merge 1 commit intoGreptimeTeam:mainfrom
Conversation
Signed-off-by: jeremyhi <fengjiachun@gmail.com>
fengjiachun
requested review from
evenyag,
v0y4g3r and
waynexia
as code owners
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
This pull request optimizes the convert_bulk_part function by pre-computing column indices to avoid repeated hash map lookups during batch processing. The changes improve performance without altering the function's behavior.
Changes:
- Pre-compute column indices for primary key, field, timestamp, and sparse primary key dictionary columns before the main processing loops
- Use
SmallVecinstead ofVecfor primary key values to avoid heap allocations for typical small primary key sizes (≤16 columns) - Pre-compute column ID and vector pairs to avoid repeated zip operations in the row processing loop
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Pre-compute column indices. | ||
| // For sparse encoding, primary key columns are not in the input schema (already encoded) | ||
| let pk_col_indices = if !is_sparse { |
There was a problem hiding this comment.
Can we add a benchmark for it to see the how much is the improvement?
| .as_str(), | ||
| ) | ||
| .context(ColumnNotFoundSnafu { | ||
| column: ®ion_metadata.time_index_column().column_schema.name, |
There was a problem hiding this comment.
nit: with_context
time_index_column has one hash loop up inside it
|
What's the status of this PR? @fengjiachun |
Will work on it (will add benchmark). |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
As @copilot said
PR Checklist
Please convert it to a draft if some of the following conditions are not met.