⚡ Bolt: [performance improvement] Optimize D1 SQL generation#215
Open
bashandbone wants to merge 1 commit intomainfrom
Open
⚡ Bolt: [performance improvement] Optimize D1 SQL generation#215bashandbone wants to merge 1 commit intomainfrom
bashandbone wants to merge 1 commit intomainfrom
Conversation
Refactored D1 SQL query generation in crates/flow/src/targets/d1.rs to utilize `String::with_capacity` and the `write!` macro instead of generating intermediate Vec collections and relying on `.join()`. This eliminates unnecessary heap allocations and string copies. 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 GuideRefactors D1 SQL generation for table and index creation to build SQL strings incrementally using pre-allocated Flow diagram for optimized create_table_sql generationflowchart TD
A[Start create_table_sql] --> B[Init sql with capacity 256]
B --> C[Append CREATE TABLE IF NOT EXISTS and table_name and opening parenthesis]
C --> D[Iterate key_columns and value_columns]
D --> E{First column?}
E -- Yes --> F[Set first to false]
E -- No --> G[Append comma and space]
G --> H[Write column name and sql_type]
F --> H[Write column name and sql_type]
H --> I{Column nullable?}
I -- No --> J[Append NOT NULL]
I -- Yes --> K[Do nothing]
J --> L{More columns?}
K --> L{More columns?}
L -- Yes --> D
L -- No --> M{Any key_columns?}
M -- No --> T[Append closing parenthesis]
M -- Yes --> N[Append comma and PRIMARY KEY and opening parenthesis]
N --> O[Iterate key_columns]
O --> P{First primary key column?}
P -- Yes --> Q[Set first_pk to false]
P -- No --> R[Append comma and space]
R --> S[Append key column name]
Q --> S[Append key column name]
S --> U{More key_columns?}
U -- Yes --> O
U -- No --> V[Append closing parenthesis for PRIMARY KEY]
V --> T[Append closing parenthesis]
T --> W[Return sql]
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 left some high level feedback:
- The
write!calls currently discard theirResultvialet _ = ...; since writing into aStringis infallible, consider using.unwrap()or a small helper to make this explicit and avoid silently ignoring potential errors (and clippy warnings). - The fixed
with_capacity(256)/with_capacity(128)values are somewhat arbitrary; you might want to derive these from the number of columns/index columns (e.g.,base + per_column * len) to avoid under- or over-allocating when tables vary significantly in size.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `write!` calls currently discard their `Result` via `let _ = ...`; since writing into a `String` is infallible, consider using `.unwrap()` or a small helper to make this explicit and avoid silently ignoring potential errors (and clippy warnings).
- The fixed `with_capacity(256)` / `with_capacity(128)` values are somewhat arbitrary; you might want to derive these from the number of columns/index columns (e.g., `base + per_column * len`) to avoid under- or over-allocating when tables vary significantly in size.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors D1 SQL generation in the Flow D1 target to reduce intermediate allocations while building CREATE TABLE and CREATE INDEX statements.
Changes:
- Rewrote
D1SetupState::create_table_sql()to build SQL incrementally usingString::with_capacity,write!, andpush_strinstead ofVec+.join(). - Rewrote
D1SetupState::create_indexes_sql()similarly to avoid allocating joined column lists.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+541
to
553
| // ⚡ Bolt: Use String::with_capacity and write! to avoid intermediate allocations | ||
| let mut sql = String::with_capacity(256); | ||
| let _ = write!(sql, "CREATE TABLE IF NOT EXISTS {} (", self.table_id.table_name); | ||
|
|
||
| let mut first = true; | ||
| for col in self.key_columns.iter().chain(self.value_columns.iter()) { | ||
| let mut col_def = format!("{} {}", col.name, col.sql_type); | ||
| if !first { | ||
| sql.push_str(", "); | ||
| } | ||
| first = false; | ||
|
|
||
| let _ = write!(sql, "{} {}", col.name, col.sql_type); | ||
| if !col.nullable { |
Comment on lines
+581
to
+588
| // ⚡ Bolt: Use String::with_capacity and write! for index SQL generation | ||
| let mut sql = String::with_capacity(128); | ||
| let unique = if idx.unique { "UNIQUE " } else { "" }; | ||
| format!( | ||
| "CREATE {}INDEX IF NOT EXISTS {} ON {} ({})", | ||
| unique, | ||
| idx.name, | ||
| self.table_id.table_name, | ||
| idx.columns.join(", ") | ||
| ) | ||
| let _ = write!( | ||
| sql, | ||
| "CREATE {}INDEX IF NOT EXISTS {} ON {} (", | ||
| unique, idx.name, self.table_id.table_name | ||
| ); |
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 D1 SQL query generation in
crates/flow/src/targets/d1.rsto utilizeString::with_capacityand thewrite!macro instead of generating intermediateVeccollections and relying on.join().🎯 Why: Generating SQL queries using
format!and mapping over slices to.collect::<Vec<_>>().join(", ")creates excessive intermediate memory allocations and string copies, which can impact performance when processing large numbers of columns or setup changes.📊 Impact: Reduces heap allocations and memory churn when computing SQL queries for table and index creation in the D1 target, leading to slightly faster initialization times for targets.
🔬 Measurement: Verify by executing
cargo test -p thread-flow --test d1_target_testsandcargo test -p thread-flow --test d1_minimal_tests, which confirms exact parity in SQL format logic for D1 exports.PR created automatically by Jules for task 13575450059674256912 started by @bashandbone
Summary by Sourcery
Optimize SQL string generation for D1 table and index creation to reduce allocations and improve performance.
Enhancements: