⚡ Bolt: [performance improvement] optimize SQL generation in D1 target#194
Open
bashandbone wants to merge 1 commit intomainfrom
Open
⚡ Bolt: [performance improvement] optimize SQL generation in D1 target#194bashandbone wants to merge 1 commit intomainfrom
bashandbone wants to merge 1 commit intomainfrom
Conversation
Refactor SQL statement generation methods in `d1.rs` (`build_upsert_stmt`, `build_delete_stmt`, `create_table_sql`, `create_indexes_sql`) to use `String::with_capacity` and `std::fmt::Write`. This avoids multiple intermediate `Vec` allocations, `.push()` calls, `.join()` loops, and `format!()` macros while achieving the exact same SQL outputs. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
Contributor
|
👋 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. |
Contributor
Reviewer's GuideOptimizes SQL generation in the D1 target by replacing format!/join!-based construction with in-place writes into pre-allocated String and Vec buffers for upsert, delete, table creation, and index creation statements, plus documenting the performance pattern in .jules/bolt.md. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The manual comma-handling patterns (using
first/first_updateflags) are repeated in several functions; consider extracting a small helper for appending comma-separated identifiers to reduce duplication and the chance of subtle off-by-one bugs. - In
build_upsert_stmt,values.fieldsis iterated twice (once for the insert columns and once for the update clauses); you could track the updated columns during the first pass (e.g., collecting references or indices) to avoid the second traversal and keep the logic more tightly coupled. - You import and use
std::fmt::Writeinside multiple functions; if this pattern becomes common in the module, consider a single module-level import and perhaps a short comment explaining why these directwrite!calls are preferred, to make the intent clearer for future maintainers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The manual comma-handling patterns (using `first`/`first_update` flags) are repeated in several functions; consider extracting a small helper for appending comma-separated identifiers to reduce duplication and the chance of subtle off-by-one bugs.
- In `build_upsert_stmt`, `values.fields` is iterated twice (once for the insert columns and once for the update clauses); you could track the updated columns during the first pass (e.g., collecting references or indices) to avoid the second traversal and keep the logic more tightly coupled.
- You import and use `std::fmt::Write` inside multiple functions; if this pattern becomes common in the module, consider a single module-level import and perhaps a short comment explaining why these direct `write!` calls are preferred, to make the intent clearer for future maintainers.
## Individual Comments
### Comment 1
<location path="crates/flow/src/targets/d1.rs" line_range="349-351" />
<code_context>
+ }
+ }
+
+ sql.push_str(") ON CONFLICT DO UPDATE SET ");
+
+ let mut first_update = true;
+ for (idx, _value) in values.fields.iter().enumerate() {
+ if let Some(value_field) = self.value_fields_schema.get(idx) {
</code_context>
<issue_to_address>
**issue:** Consider handling the case where there are no updatable value columns to avoid generating invalid SQL.
Currently, if `values.fields` is empty or none match `self.value_fields_schema`, `first_update` remains `true` and the SQL ends with `ON CONFLICT DO UPDATE SET` and no assignments, which will fail at runtime. To avoid this, consider either switching to `ON CONFLICT DO NOTHING` when there are no mutable columns, or returning an error if an upsert without updatable fields is invalid in this context.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+349
to
+351
| sql.push_str(") ON CONFLICT DO UPDATE SET "); | ||
|
|
||
| let mut first_update = true; |
Contributor
There was a problem hiding this comment.
issue: Consider handling the case where there are no updatable value columns to avoid generating invalid SQL.
Currently, if values.fields is empty or none match self.value_fields_schema, first_update remains true and the SQL ends with ON CONFLICT DO UPDATE SET and no assignments, which will fail at runtime. To avoid this, consider either switching to ON CONFLICT DO NOTHING when there are no mutable columns, or returning an error if an upsert without updatable fields is invalid in this context.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the D1 target’s SQL string construction to reduce allocation churn in a hot path. It focuses on statement generation inside thread-flow’s Cloudflare D1 export target, which is used during bulk indexing and incremental export work.
Changes:
- Reworked upsert/delete SQL builders in
crates/flow/src/targets/d1.rsto write directly into pre-allocatedString/Vecbuffers. - Reworked D1 setup SQL generation (
CREATE TABLE/CREATE INDEX) to avoid intermediateformat!andjoin()allocations. - Added an internal
.julesnote documenting the SQL-generation optimization pattern and reported benchmark impact.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
crates/flow/src/targets/d1.rs |
Refactors runtime and setup SQL generation for the D1 export target to reduce allocations. |
.jules/bolt.md |
Records the optimization as an internal performance learning note. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+316
to
+323
| for (idx, key_field) in self.key_fields_schema.iter().enumerate() { | ||
| if let Some(key_part) = key.0.get(idx) { | ||
| columns.push(self.key_fields_schema[idx].name.clone()); | ||
| placeholders.push("?".to_string()); | ||
| if !first { | ||
| sql.push_str(", "); | ||
| } | ||
| sql.push_str(&key_field.name); | ||
| params.push(key_part_to_json(key_part)?); | ||
| first = false; |
Comment on lines
328
to
+335
| for (idx, value) in values.fields.iter().enumerate() { | ||
| if let Some(value_field) = self.value_fields_schema.get(idx) { | ||
| columns.push(value_field.name.clone()); | ||
| placeholders.push("?".to_string()); | ||
| if !first { | ||
| sql.push_str(", "); | ||
| } | ||
| sql.push_str(&value_field.name); | ||
| params.push(value_to_json(value)?); | ||
| update_clauses.push(format!( | ||
| "{} = excluded.{}", | ||
| value_field.name, value_field.name | ||
| )); | ||
| first = false; |
Comment on lines
380
to
+387
| for (idx, _key_field) in self.key_fields_schema.iter().enumerate() { | ||
| if let Some(key_part) = key.0.get(idx) { | ||
| where_clauses.push(format!("{} = ?", self.key_fields_schema[idx].name)); | ||
| if !first { | ||
| sql.push_str(" AND "); | ||
| } | ||
| write!(sql, "{} = ?", self.key_fields_schema[idx].name).unwrap(); | ||
| params.push(key_part_to_json(key_part)?); | ||
| first = false; |
Comment on lines
+349
to
361
| sql.push_str(") ON CONFLICT DO UPDATE SET "); | ||
|
|
||
| let mut first_update = true; | ||
| for (idx, _value) in values.fields.iter().enumerate() { | ||
| if let Some(value_field) = self.value_fields_schema.get(idx) { | ||
| if !first_update { | ||
| sql.push_str(", "); | ||
| } | ||
| write!(sql, "{} = excluded.{}", value_field.name, value_field.name).unwrap(); | ||
| first_update = false; | ||
| } | ||
| } | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 What:
Refactored SQL generation in
crates/flow/src/targets/d1.rs(specifically inbuild_upsert_stmt,build_delete_stmt,create_table_sql, andcreate_indexes_sql). I replacedformat!macros and intermediateVec<String>::join()operations with directwrite!formatting into pre-allocated buffers (String::with_capacityandVec::with_capacity).🎯 Why:
The D1 target handles massive batches of upserts during index construction. Re-allocating intermediate strings and parameter collections for every column inside a loop was creating severe memory churn and causing O(N) allocations for every statement, becoming a bottleneck in batch operations.
📊 Impact:
build_upsert_stmt: ~72% reduction in generation latency (1.76 µs -> 494 ns).build_10_upsert_statements: ~75% reduction in latency (24 µs -> 4.9 µs).build_delete_stmt: ~57% reduction in latency (500 ns -> 212 ns).🔬 Measurement:
Run the benchmark directly using:
cargo bench -p thread-flow --bench d1_profiling -- statement_generation.PR created automatically by Jules for task 11748465007323709970 started by @bashandbone
Summary by Sourcery
Optimize SQL generation performance for the D1 target by reducing allocations during statement construction.
Enhancements: