⚡ Bolt: [performance improvement] Optimize D1 query generation#236
⚡ Bolt: [performance improvement] Optimize D1 query generation#236bashandbone wants to merge 1 commit into
Conversation
This replaces intermediate `Vec<String>` allocations with `String::with_capacity` and `write!` macro in the `build_upsert_stmt` and `build_delete_stmt` functions within `crates/flow/src/targets/d1.rs`. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideRefactors D1 upsert/delete SQL generation to build queries directly into pre-allocated Strings using fmt::Write, reducing intermediate allocations, and makes a few minor formatting/closure-style cleanups elsewhere in the codebase. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
build_upsert_stmt, the number ofVALUESplaceholders is derived fromparams.len(), which implicitly assumes all params are for the insert columns only; consider tracking a dedicated placeholder count so this logic remains correct if additional parameters (e.g., for WHERE/RETURNING) are introduced in the future. - The
write!calls on aStringcannot realistically fail except for formatting issues; if you don't expect dynamic formatting errors, usingexpector an internal helper to avoid repetitivemap_err(|e| RecocoError::internal_msg(e.to_string()))boilerplate could simplify the control flow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `build_upsert_stmt`, the number of `VALUES` placeholders is derived from `params.len()`, which implicitly assumes all params are for the insert columns only; consider tracking a dedicated placeholder count so this logic remains correct if additional parameters (e.g., for WHERE/RETURNING) are introduced in the future.
- The `write!` calls on a `String` cannot realistically fail except for formatting issues; if you don't expect dynamic formatting errors, using `expect` or an internal helper to avoid repetitive `map_err(|e| RecocoError::internal_msg(e.to_string()))` boilerplate could simplify the control flow.
## Individual Comments
### Comment 1
<location path="crates/flow/src/targets/d1.rs" line_range="347-350" />
<code_context>
+ sql.push('?');
+ }
+
+ sql.push_str(") ON CONFLICT DO UPDATE SET ");
+
+ first = true;
+ for (idx, _value) in values.fields.iter().enumerate() {
+ if let Some(value_field) = self.value_fields_schema.get(idx) {
+ if !first {
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle the case where there are no value fields to update in the `ON CONFLICT` clause
If `values.fields` contains no `value_field`s (e.g. only key columns or an empty payload), this will emit `... ON CONFLICT DO UPDATE SET ` with nothing after `SET`, which is invalid SQL. Since you’re already refactoring, consider guarding this case by falling back to `ON CONFLICT DO NOTHING` or omitting the `ON CONFLICT` clause when there are no updatable columns.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| sql.push_str(") ON CONFLICT DO UPDATE SET "); | ||
|
|
||
| first = true; | ||
| for (idx, _value) in values.fields.iter().enumerate() { |
There was a problem hiding this comment.
issue (bug_risk): Handle the case where there are no value fields to update in the ON CONFLICT clause
If values.fields contains no value_fields (e.g. only key columns or an empty payload), this will emit ... ON CONFLICT DO UPDATE SET with nothing after SET, which is invalid SQL. Since you’re already refactoring, consider guarding this case by falling back to ON CONFLICT DO NOTHING or omitting the ON CONFLICT clause when there are no updatable columns.
There was a problem hiding this comment.
Pull request overview
This PR optimizes D1 SQL statement generation by replacing temporary vector/string joins with direct preallocated String construction while preserving positional parameter ordering.
Changes:
- Builds D1 upsert/delete SQL using
String::with_capacityandwrite!. - Preallocates parameter vectors based on schema/value sizes.
- Applies rustfmt-only formatting changes in unrelated rule/tree-sitter files.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
crates/flow/src/targets/d1.rs |
Optimizes D1 upsert and delete SQL builders. |
crates/rule-engine/src/rule/referent_rule.rs |
Formatting-only change. |
crates/rule-engine/src/rule/mod.rs |
Formatting-only change. |
crates/ast-engine/src/tree_sitter/mod.rs |
Formatting-only changes in string handling and a test assertion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
💡 What: Replaced intermediate vector allocations (
columns,placeholders,update_clauses,where_clauses) in D1 query builders (build_upsert_stmt,build_delete_stmt) with a pre-allocatedStringusingString::with_capacityandstd::fmt::Write.🎯 Why: SQL query generation happens frequently during D1 target synchronization. The previous implementation used multiple
Vec<String>allocations andformat!/joinoperations per query, causing unnecessary memory allocation and string copying overhead.📊 Impact: Significantly reduces heap allocations and string formatting overhead for every D1 upsert and delete statement generated, leading to faster execution and less GC/memory churn during export operations.
🔬 Measurement: Verified by running formatting, linting, and targeted D1 target tests (
cargo test -p thread-flow --test d1_target_tests --test d1_minimal_tests --test d1_cache_integration), ensuring no regressions and that the generated SQL exactly matches the previous logic.PR created automatically by Jules for task 12786738793941327599 started by @bashandbone
Summary by Sourcery
Optimize D1 SQL upsert and delete statement generation to reduce allocations and improve performance during target synchronization.
Enhancements: