CORE-1210: optimize insert_rows / render_row_to_sql Jinja path (Fusion artifact upload slowdown)#1041
CORE-1210: optimize insert_rows / render_row_to_sql Jinja path (Fusion artifact upload slowdown)#1041haritamar wants to merge 3 commits into
Conversation
Hoist per-column work out of the per-row loop, drop per-row duration measuring and case-insensitive dict lookups, resolve adapter.dispatch once per insert, and build chunk queries via list join instead of growing-string concat. Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
👋 @haritamar |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe insert-row query generator now caches rendering metadata and dependencies, renders rows through helper macros, and assembles chunked SQL statements. Integration tests cover SQL literal formatting, JSON serialization, combined rows, and row-level chunking. ChangesInsert row rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant dbt_operation
participant get_insert_rows_queries
participant render_row_to_sql
participant render_value
dbt_operation->>get_insert_rows_queries: provide columns, rows, and chunk settings
get_insert_rows_queries->>render_row_to_sql: render each row with cached metadata
render_row_to_sql->>render_value: convert column values to SQL literals
get_insert_rows_queries-->>dbt_operation: return chunked insert queries
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration_tests/tests/test_dbt_artifacts/test_insert_rows.py`:
- Line 22: Update the assertions in the test around the query checks at lines 22
and 39 to derive the expected escaped O’Brien value using the active adapter’s
escaping behavior, rather than hardcoding SQL quote-doubling. Ensure the
expectation matches elementary.escape_special_chars for each warehouse in the
matrix.
In `@macros/utils/table_operations/insert_rows.sql`:
- Around line 215-239: Update get_row_render_metadata and its callers so row_key
resolution is performed for each row rather than derived once from rows[0] and
reused across the batch. Preserve the existing exact, lowercase, and uppercase
key matching for every row, ensuring mixed key casing does not produce null
values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4185b170-4b46-4600-b449-ceaaa53d6168
📒 Files selected for processing (3)
integration_tests/dbt_project/macros/test_render_insert_rows_queries.sqlintegration_tests/tests/test_dbt_artifacts/test_insert_rows.pymacros/utils/table_operations/insert_rows.sql
| {% macro get_row_render_metadata(columns, sample_row) %} | ||
| {% set column_meta = [] %} | ||
| {% for column in columns %} | ||
| {% set is_created_at = column.name | lower == "created_at" %} | ||
| {% set row_key = none %} | ||
| {% if not is_created_at and sample_row is not none %} | ||
| {% if column.name in sample_row %} {% set row_key = column.name %} | ||
| {% elif column.name | lower in sample_row %} | ||
| {% set row_key = column.name | lower %} | ||
| {% elif column.name | upper in sample_row %} | ||
| {% set row_key = column.name | upper %} | ||
| {% endif %} | ||
| {% endif %} | ||
| {% do column_meta.append( | ||
| { | ||
| "is_created_at": is_created_at, | ||
| "row_key": row_key, | ||
| "normalized_type": elementary.normalize_data_type( | ||
| column.dtype | ||
| ), | ||
| } | ||
| ) %} | ||
| {% endfor %} | ||
| {% do return(column_meta) %} | ||
| {% endmacro %} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether row-key casing is guaranteed consistent by all row-producing
# (flatten) macros that feed into insert_rows/get_insert_rows_queries.
rg -n 'insensitive_get_dict_value' --type sql -C2
rg -n 'def flatten|macro flatten' --type sql -C3
rg -n 'get_insert_rows_queries|insert_rows\(' --type sql -g '!macros/utils/table_operations/insert_rows.sql'Repository: elementary-data/dbt-data-reliability
Length of output: 174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- insert_rows.sql outline ---'
ast-grep outline macros/utils/table_operations/insert_rows.sql --view expanded || true
echo
echo '--- relevant search ---'
rg -n --hidden --glob '!**/.git/**' \
'insensitive_get_dict_value|get_row_render_metadata|render_row_to_sql|get_insert_rows_queries|flatten' \
macros . || trueRepository: elementary-data/dbt-data-reliability
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file size ---'
wc -l macros/utils/table_operations/insert_rows.sql
echo
echo '--- around referenced lines ---'
sed -n '1,320p' macros/utils/table_operations/insert_rows.sql | nl -ba | sed -n '180,280p'
echo
echo '--- callers and helpers ---'
rg -n --hidden --glob '!**/.git/**' \
'get_insert_rows_queries|render_row_to_sql|insensitive_get_dict_value|sample_row|row_key|flatten' \
macrosRepository: elementary-data/dbt-data-reliability
Length of output: 315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- insert_rows.sql (lines 101-280) ---'
sed -n '101,280p' macros/utils/table_operations/insert_rows.sql
echo
echo '--- insensitive_get_dict_value.sql ---'
cat -n macros/utils/dict_utils/insensitive_get_dict_value.sqlRepository: elementary-data/dbt-data-reliability
Length of output: 7997
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- callers of get_insert_rows_queries / insert_rows ---'
rg -n --hidden --glob '!**/.git/**' 'get_insert_rows_queries\(|insert_rows\(' macros
echo
echo '--- top of insert_rows.sql ---'
sed -n '1,120p' macros/utils/table_operations/insert_rows.sql
echo
echo '--- flatten_* macros that return list/dict rows ---'
rg -n --hidden --glob '!**/.git/**' 'return\(.*flatten|do return\(flatten|append\(flat|extend\(flatten' macros/edr macros/utilsRepository: elementary-data/dbt-data-reliability
Length of output: 7730
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- upload_artifacts_to_table.sql ---'
sed -n '1,140p' macros/edr/dbt_artifacts/upload_artifacts_to_table.sql
echo
echo '--- upload_dbt_invocation.sql ---'
sed -n '1,140p' macros/edr/dbt_artifacts/upload_dbt_invocation.sql
echo
echo '--- handle_tests_results.sql ---'
sed -n '1,120p' macros/edr/tests/on_run_end/handle_tests_results.sql
echo
echo '--- create_model_baseline_table.sql ---'
sed -n '1,120p' macros/edr/tests/test_utils/create_model_baseline_table.sql
echo
echo '--- replace_table_data.sql ---'
sed -n '1,220p' macros/utils/table_operations/replace_table_data.sqlRepository: elementary-data/dbt-data-reliability
Length of output: 22458
Keep key lookup independent of the first row. row_key is derived from rows[0] and reused for every row, so any batch that mixes key casing will silently write null for later rows. Normalize per row or make the casing invariant explicit at the call sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@macros/utils/table_operations/insert_rows.sql` around lines 215 - 239, Update
get_row_render_metadata and its callers so row_key resolution is performed for
each row rather than derived once from rows[0] and reused across the batch.
Preserve the existing exact, lowercase, and uppercase key matching for every
row, ensuring mixed key casing does not produce null values.
There was a problem hiding this comment.
Keeping the one-time key map here intentionally. Every caller of get_insert_rows_queries feeds rows produced by the flatten_* artifact macros (e.g. flatten_model, flatten_column, flatten_test), which emit a fixed, literal set of keys — the same key casing for every row in a batch. Because the keys are consistent across rows, resolving row_key once from the first row is equivalent to the old per-row insensitive_get_dict_value, and resolving it once is exactly what CORE-1210 asks for (the per-cell case-insensitive lookup was a large part of the Fusion slowdown). Reintroducing a per-row/per-cell 3-way casing check would re-add the FFI cost the ticket is trying to remove. If a future caller ever passes rows with heterogeneous key casing this would need revisiting, but that isn't possible with the current flatten-based inputs.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
aside - CodeRabbit - does the above response make sense?
There was a problem hiding this comment.
@haritamar, yes—the response makes sense for the current call paths and the stated CORE-1210 performance goal.
If all inputs are produced by the flatten_* macros and those macros guarantee the same literal key casing for every row in a batch, then deriving row_key from the first row is behaviorally equivalent to the previous per-cell case-insensitive lookup. Reintroducing that lookup in the hot loop would undermine the optimization this PR is specifically targeting.
The only caveat is API scope: get_insert_rows_queries itself does not enforce that invariant, so a future caller passing heterogeneous key casing could still get null values. That is an acceptable trade-off if the macro is intentionally internal / constrained to flatten-generated rows. A short contract comment—as already present—helps make that assumption explicit.
So I would treat the original finding as not actionable for this PR, rather than requiring a code change.
✏️ Learnings added
Learnt from: haritamar
Repo: elementary-data/dbt-data-reliability PR: 1041
File: macros/utils/table_operations/insert_rows.sql:215-239
Timestamp: 2026-07-23T17:39:04.799Z
Learning: In `macros/utils/table_operations/insert_rows.sql`, `elementary.get_insert_rows_queries` currently receives rows generated by `flatten_*` artifact macros, whose dictionaries use a fixed, consistent key casing within each batch. Therefore, `elementary.get_row_render_metadata` intentionally resolves each column key once from the first row to avoid per-cell case-insensitive lookup overhead in the CORE-1210 performance-sensitive insert-rendering path.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
escape_special_chars differs per warehouse ('' vs backslash), so derive
the expected quoted literal from the adapter instead of hardcoding the
Postgres-style doubled quote.
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
| loop only does a dict lookup + value render. `sample_row` (the first row) is | ||
| used to resolve the actual dict key for each column, replacing the per-cell | ||
| case-insensitive lookup - flatten macros emit consistent keys across rows. #} | ||
| {% macro get_row_render_metadata(columns, sample_row) %} |
There was a problem hiding this comment.
should it be get_columns_metadata?
also I think the comment can be removed - it's pretty clear what this macro does.
There was a problem hiding this comment.
Renamed to get_columns_metadata and removed the comment. Done in d84117a.
| {{- adapter.dispatch("render_value", "elementary")(value, data_type) -}} | ||
| {# `escaper` lets callers pass a pre-resolved escape_special_chars implementation | ||
| so the hot insert path avoids an adapter.dispatch per rendered cell. When it's | ||
| not provided (other callers) it is resolved once here, matching prior behavior. #} |
There was a problem hiding this comment.
Remove "matching prior behavior", don't document past code
| {# Accumulate each chunk's rendered rows in a list and join once when the | ||
| chunk is finalized, instead of repeatedly concatenating a growing query | ||
| string (which is O(n^2)). A namespace is required so reassignments made | ||
| inside the loop persist across iterations. #} |
There was a problem hiding this comment.
remove this comment
| {% set current_chunk_size = namespace(data=0) %} | ||
| {# Precompute per-column rendering metadata and adapter implementations once, | ||
| hoisted out of the per-row loop (dtypes / created_at handling / dispatch | ||
| results are identical for every row). #} |
There was a problem hiding this comment.
remove this comment
…ments Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
|
|
||
| {% set current_query = namespace(data=base_insert_query) %} | ||
| {% set current_chunk_size = namespace(data=0) %} | ||
| {% set column_meta = elementary.get_columns_metadata( |
There was a problem hiding this comment.
aside - getting column metadata once, instead of re-computing it over and over on every row
| {% endfor %} | ||
| {% set row_sql = "({})".format(rendered_column_values | join(",")) %} | ||
|
|
||
| {% do elementary.end_duration_measure_context("render_row_to_sql") %} |
There was a problem hiding this comment.
aside - These begin/end measurements were very expensive by themselves, so we removed them
| {% if column.is_created_at %} | ||
| {% do rendered_column_values.append(created_at_sql) %} | ||
| {% else %} | ||
| {% set column_value = elementary.insensitive_get_dict_value( |
There was a problem hiding this comment.
aside - insensitive_get_dict_value replaced with a simple get, as the flattened rows already lower all keys
| ) %} | ||
| {% set created_at_sql = elementary.edr_current_timestamp() %} | ||
| {% set render_value_impl = adapter.dispatch("render_value", "elementary") %} | ||
| {% set escaper = adapter.dispatch("escape_special_chars", "elementary") %} |
There was a problem hiding this comment.
aside - smaller optimization - pre-compute once and then pass adapter.dispatch to avoid doing this logic on every row.
| {% for row in rows %} | ||
| {% set row_sql = elementary.render_row_to_sql(row, columns) %} | ||
| {% set query_with_row = ( | ||
| current_query.data + ("," if not loop.first else "") + row_sql |
There was a problem hiding this comment.
aside - replaced this with collecting an array of all the rows in the chunk and joining once (makes this O(n) instead of O(n^2))
1 participant
Summary
Artifact upload (
DBT_COLUMNS, ~65k rows) spends minutes in Jinja buildingINSERT … VALUES, dramatically worse under dbt Fusion (MiniJinja FFI overhead per macro/dict boundary). Customer trace: ~8m15s inrender_row_to_sqlacross 65,126 rows while the warehouse inserts took only ~45s.This reworks the hot path in
macros/utils/table_operations/insert_rows.sqlso the per-row loop does only a dict lookup + value render, moving all repeated work out of the loop. No change to the emitted SQL shape (stillINSERT … VALUES) or to insert semantics across adapters.What changed
(1) Remove per-row duration instrumentation.
render_row_to_sqlno longer callsbegin/end_duration_measure_context(was ~65k stack push/pop +utcnow()+ long name concat per row). Measurement stays atget_insert_rows_queries/insert_rowslevel.(2) Hoist column-level work out of the loop into a new
get_row_render_metadata(columns, sample_row), computed once per insert:This removes ~900k
normalize_data_type → data_type_listadapter.dispatchcalls for the customer case.(3) Drop
insensitive_get_dict_valuein this path.row_keyis resolved once (exact / lower / upper against the first row — flatten emits consistent keys), then the loop does a directrow.get(row_key).(4) Resolve
adapter.dispatchonce per insert.render_valueandescape_special_charsimpls are resolved once and reused.render_value/default__render_valuegained an optionalescaperparam so the pre-resolved escaper is threaded through instead of re-dispatching per string cell (default behavior unchanged whenescaperis omitted, for other callers).(5) Fix quadratic chunk assembly. Instead of
current_query.data + "," + row_sqlon a growing multi-MB string, each chunk accumulates row SQL in a list and joins once:Length tracking is done incrementally (namespace counter) so no giant string is rebuilt to check
query_max_size. Existing chunking semantics preserved:query_max_size,chunk_size, andon_query_exceedoversized-row handling.Correctness / tests
integration_tests/tests/test_dbt_artifactssuite passes on Postgres (46 passed, 4 skipped), includingtest_artifacts_collection_in_multiple_row_batches(forces smallquery_max_size→ multiple chunks).test_insert_rows.py+test_render_insert_rows_queriesmacro covering the rendered VALUES output: single-quote escaping, numbers, nulls, nested mapping/sequence viatojson, and chunk splitting.Link to Devin session: https://app.devin.ai/sessions/11d70ed2cdfe4ae182c112e4135de8d3
Requested by: @haritamar
Summary by CodeRabbit
New Features
Bug Fixes
Tests