⚡ Bolt: [D1 SQL generation optimization]#203
Open
bashandbone wants to merge 1 commit intomainfrom
Open
Conversation
…queries Replaces intermediate string `Vec` allocations and `.join()` operations with pre-allocated `String` buffers (`String::with_capacity`) and incremental `write!` formatting in `build_upsert_stmt` and `build_delete_stmt` to lower heap allocations during bulk operations. Co-authored-by: bashandbone <[email protected]>
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 D1 SQL statement generation by replacing intermediate Vec assemblies and format!/join calls with preallocated String buffers and std::fmt::Write, plus a few minor style/formatting cleanups in other crates and an addition to the Bolt performance notes. Class diagram for optimized D1ExportContext SQL generationclassDiagram
class D1ExportContext {
String table_name
KeyFieldSchema[] key_fields_schema
ValueFieldSchema[] value_fields_schema
+build_upsert_stmt(key: KeyValue, values: FieldValues) Result~(String, Vec_serde_json_Value_) , RecocoError~
+build_delete_stmt(key: KeyValue) Result~(String, Vec_serde_json_Value_) , RecocoError~
}
class KeyValue {
KeyPart[] 0
}
class FieldValues {
ValueField[] fields
}
class KeyFieldSchema {
String name
}
class ValueFieldSchema {
String name
}
class KeyPart
class ValueField
class serde_json_Value
class RecocoError
D1ExportContext --> KeyValue : uses
D1ExportContext --> FieldValues : uses
D1ExportContext --> serde_json_Value : params
D1ExportContext --> RecocoError : error
D1ExportContext --> KeyFieldSchema : reads
D1ExportContext --> ValueFieldSchema : reads
KeyValue --> KeyPart : wraps
FieldValues --> ValueField : contains
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 SQL builders now rely on multiple
write!(...).unwrap()calls; sincefmt::WriteforStringis infallible you can either ignore theResultor useexpectonce in a small helper to avoid scatteringunwrap()s and make panics more intentional. - The new entry in
.jules/bolt.mdembeds literal\ncharacters inside the markdown line, which will render oddly; consider splitting this into actual newlines so the “Learning” and “Action” sections format consistently with the previous entry.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The SQL builders now rely on multiple `write!(...).unwrap()` calls; since `fmt::Write` for `String` is infallible you can either ignore the `Result` or use `expect` once in a small helper to avoid scattering `unwrap()`s and make panics more intentional.
- The new entry in `.jules/bolt.md` embeds literal `\n` characters inside the markdown line, which will render oddly; consider splitting this into actual newlines so the “Learning” and “Action” sections format consistently with the previous entry.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
Optimizes Cloudflare D1 target SQL generation by reducing intermediate allocations during upsert/delete statement construction in thread-flow, aiming to improve performance in bulk export loops.
Changes:
- Reworked
build_upsert_stmtandbuild_delete_stmtto build SQL via pre-allocatedStringbuffers and incrementalwrite!calls (avoidsVec<String>+.join()allocations). - Minor formatting-only refactors in rule-engine and ast-engine code.
- Added a performance “Bolt” log entry describing the optimization.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rule-engine/src/rule/referent_rule.rs | Formatting-only refactor (no behavioral change). |
| crates/rule-engine/src/rule/mod.rs | Formatting-only refactor (no behavioral change). |
| crates/flow/src/targets/d1.rs | Implements allocation-reduced SQL string assembly for upsert/delete. |
| crates/ast-engine/src/tree_sitter/mod.rs | Formatting-only refactor (no behavioral change). |
| .jules/bolt.md | Adds a learning-log entry for the SQL generation pre-allocation change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## 2026-04-08 - [Performance: Defer Allocation during Traversal] | ||
| **Learning:** During DAG traversals, creating owned variants of identifiers (like `file.to_path_buf()`) *before* checking `visited` HashSets results in heap allocations (O(E)) for every edge instead of every visited node (O(V)). By moving the `&PathBuf` allocation strictly *after* all HashSet `contains` checks using the borrowed reference (`&Path`), we drastically reduce memory churn. | ||
| **Action:** Always check `HashSet::contains` with a borrowed reference *before* creating the owned version required by `HashSet::insert`, especially in performance-critical graph traversal paths. | ||
| ## 2026-05-07 - [Performance: SQL Generation Pre-allocation]\n**Learning:** Generating SQL string with intermediate `vec![]` string arrays, pushing `format!` results, and calling `.join(",")` incurs unnecessary string and heap allocations, especially during bulk upsert or deletes.\n**Action:** Use pre-allocated Strings with `String::with_capacity` and leverage `std::fmt::Write` directly (`write!`) to assemble complex SQL queries with no intermediate array joining. |
Comment on lines
28
to
31
| impl<R> Registration<R> { | ||
| fn read(&self) -> Arc<RapidMap<String, R>> { | ||
| self.0 | ||
| .read() | ||
| .unwrap_or_else(|e| e.into_inner()) | ||
| .clone() | ||
| self.0.read().unwrap_or_else(|e| e.into_inner()).clone() | ||
| } |
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: Replaced intermediate string vector allocations (
columns,placeholders,update_clauses) and.join()calls with pre-allocatedStringbuffers (usingString::with_capacity) and incrementalstd::fmt::Writelogic insidebuild_upsert_stmtandbuild_delete_stmtincrates/flow/src/targets/d1.rs.🎯 Why: The original implementation caused multiple string heap allocations per query loop (via
format!()) and further allocation overhead when executing.join(", "). This adds up especially when doing bulk processing.📊 Impact: Significantly reduces O(N) memory allocations per target SQL string generated, lowering garbage and improving loop efficiency.
🔬 Measurement: Verified with
cargo test -p thread-flow --test d1_target_tests. Code correctness holds true and intermediate memory allocations are deferred.PR created automatically by Jules for task 10287299905955204933 started by @bashandbone
Summary by Sourcery
Optimize SQL statement generation for D1 targets to reduce string allocations and capture the performance pattern in project documentation.
Enhancements:
Documentation: