fix: rename get_incremental_microbatch_sql to capture_microbatch_compiled_code_sql#1024
Conversation
…eaking microbatch for all Elementary users
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe public dbt macro ChangesMacro rename:
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
👋 @GuyEshdat |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
macros/edr/dbt_artifacts/microbatch/capture_microbatch_compiled_code.sql (1)
1-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCommit the
sqlfmtoutput for this macro.CI is failing because
sqlfmtrewrote this SQL file, so the checked-in version is not formatter-clean yet.🤖 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/edr/dbt_artifacts/microbatch/capture_microbatch_compiled_code.sql` around lines 1 - 49, Run sqlfmt on the capture_microbatch_compiled_code.sql file to apply the SQL formatter's required formatting changes to both the capture_microbatch_compiled_code_sql macro and the capture_microbatch_compiled_code_for_model macro, then commit the formatted output. This will ensure the checked-in version matches the formatter-clean standards that CI expects.Source: Pipeline failures
🤖 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_microbatch_compiled_code.py`:
- Line 103: Add a new test case to cover the regression scenario where
Elementary is installed without a get_incremental_microbatch_sql macro override
and without the require_batched_execution_for_custom_microbatch_strategy
behavior flag enabled. This test should either execute without the
_with_microbatch_macro_file context manager or explicitly disable the
require_batched_execution_for_custom_microbatch_strategy flag to simulate the
default-flag scenario. This ensures the original regression (which occurred in
this specific no-wrapper/default-flag configuration) is properly covered and
prevented from happening again.
---
Outside diff comments:
In `@macros/edr/dbt_artifacts/microbatch/capture_microbatch_compiled_code.sql`:
- Around line 1-49: Run sqlfmt on the capture_microbatch_compiled_code.sql file
to apply the SQL formatter's required formatting changes to both the
capture_microbatch_compiled_code_sql macro and the
capture_microbatch_compiled_code_for_model macro, then commit the formatted
output. This will ensure the checked-in version matches the formatter-clean
standards that CI expects.
🪄 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: 1d40cc0b-e8be-4195-a039-196954e6b63b
📒 Files selected for processing (2)
integration_tests/tests/test_dbt_artifacts/test_microbatch_compiled_code.pymacros/edr/dbt_artifacts/microbatch/capture_microbatch_compiled_code.sql
| macro_sql = """ | ||
| {% macro __MACRO_NAME__(arg_dict) %} | ||
| {{ return(elementary.get_incremental_microbatch_sql(arg_dict)) }} | ||
| {{ return(elementary.capture_microbatch_compiled_code_sql(arg_dict)) }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect whether microbatch compiled-code tests cover the no-wrapper/default-flag regression path.
# Expected: at least one test path that does not enter _with_microbatch_macro_file and runs with
# require_batched_execution_for_custom_microbatch_strategy unset or explicitly False.
fd -p 'test_microbatch_compiled_code.py' -x sed -n '1,220p' {}
rg -n -C4 \
'require_batched_execution_for_custom_microbatch_strategy|_with_microbatch_macro_file|capture_microbatch_compiled_code_sql|get_incremental_microbatch_sql' \
--iglob '*.py' --iglob '*.yml' --iglob '*.yaml'Repository: elementary-data/dbt-data-reliability
Length of output: 8474
🏁 Script executed:
wc -l integration_tests/tests/test_dbt_artifacts/test_microbatch_compiled_code.pyRepository: elementary-data/dbt-data-reliability
Length of output: 160
🏁 Script executed:
# Check if there are other test functions in the file
rg -n 'def test_' integration_tests/tests/test_dbt_artifacts/test_microbatch_compiled_code.pyRepository: elementary-data/dbt-data-reliability
Length of output: 140
🏁 Script executed:
# Check if the behavior flag is overridden or disabled anywhere in the test file or fixtures
rg -n 'require_batched_execution_for_custom_microbatch_strategy' integration_tests/tests/test_dbt_artifacts/ -A 3 -B 3Repository: elementary-data/dbt-data-reliability
Length of output: 62
🏁 Script executed:
# Search for require_batched_execution_for_custom_microbatch_strategy across the entire repo
rg 'require_batched_execution_for_custom_microbatch_strategy' --type yaml --type py | head -20Repository: elementary-data/dbt-data-reliability
Length of output: 191
🏁 Script executed:
# Check the conftest.py to see how dbt_project fixture is configured
fd -p 'conftest.py' -path '*/tests/*' -x head -100 {}Repository: elementary-data/dbt-data-reliability
Length of output: 249
🏁 Script executed:
# Find and check conftest files
find integration_tests -name 'conftest.py' | head -5Repository: elementary-data/dbt-data-reliability
Length of output: 116
🏁 Script executed:
# Check the entire dbt_project.yml to understand the baseline configuration
cat integration_tests/dbt_project/dbt_project.yml | grep -A 5 -B 5 'require_batched_execution_for_custom_microbatch_strategy'Repository: elementary-data/dbt-data-reliability
Length of output: 318
🏁 Script executed:
# Check if there are any test fixtures that might disable the flag
rg 'disable.*microbatch|behavior.*flag|require_batched' integration_tests/tests/ --type py -B 3 -A 3Repository: elementary-data/dbt-data-reliability
Length of output: 62
🏁 Script executed:
# Check the test conftest.py for any flag overrides or fixture setup
cat integration_tests/tests/conftest.py | head -150Repository: elementary-data/dbt-data-reliability
Length of output: 4911
🏁 Script executed:
# Look for any pytest markers or parametrize that might disable the flag
rg -n 'dbt_runner|vars|disable|flags' integration_tests/tests/test_dbt_artifacts/test_microbatch_compiled_code.pyRepository: elementary-data/dbt-data-reliability
Length of output: 229
🏁 Script executed:
# Check the complete test file to see all content after line 150
tail -20 integration_tests/tests/test_dbt_artifacts/test_microbatch_compiled_code.pyRepository: elementary-data/dbt-data-reliability
Length of output: 850
🏁 Script executed:
# Check if there are any fixtures in conftest that set dbt vars or flags for microbatch tests
rg -n 'dbt_project|fixture' integration_tests/tests/conftest.py | head -30Repository: elementary-data/dbt-data-reliability
Length of output: 974
🏁 Script executed:
# Check the dbt_project fixture definition to understand baseline setup
rg -A 30 'def dbt_project' integration_tests/tests/conftest.pyRepository: elementary-data/dbt-data-reliability
Length of output: 1077
🏁 Script executed:
# Verify the actual line 103 content in the test file to confirm the function call
sed -n '100,110p' integration_tests/tests/test_dbt_artifacts/test_microbatch_compiled_code.pyRepository: elementary-data/dbt-data-reliability
Length of output: 472
Add regression coverage for the no-wrapper/default-flag scenario.
The test currently covers two paths via _with_microbatch_macro_file: one where the macro name matches dbt's call (compiled_code populated) and one where it doesn't (compiled_code empty). However, both paths execute with a macro file present and with require_batched_execution_for_custom_microbatch_strategy set to True globally.
The original regression occurred when Elementary was installed without a get_incremental_microbatch_sql macro override AND without the behavior flag forcing batches. Add a test case that runs without entering the macro file context manager and/or with the behavior flag disabled to prevent this regression from being missed again.
🤖 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 `@integration_tests/tests/test_dbt_artifacts/test_microbatch_compiled_code.py`
at line 103, Add a new test case to cover the regression scenario where
Elementary is installed without a get_incremental_microbatch_sql macro override
and without the require_batched_execution_for_custom_microbatch_strategy
behavior flag enabled. This test should either execute without the
_with_microbatch_macro_file context manager or explicitly disable the
require_batched_execution_for_custom_microbatch_strategy flag to simulate the
default-flag scenario. This ensures the original regression (which occurred in
this specific no-wrapper/default-flag configuration) is properly covered and
prevented from happening again.
2 participants
Problem
Installing Elementary inadvertently broke microbatch execution for all users, even those who never set up the compiled code capture feature.
Root cause: dbt 1.9+ has two separate lookups for the
get_incremental_microbatch_sqlmacro:_microbatch_macro_is_core()check — searches ALL packages. If it finds the macro anywhere with non-Core locality, it returnsFalse, causinguse_microbatch_batches()to returnFalse.Because Elementary's package defined
get_incremental_microbatch_sql(unprefixed), the_microbatch_macro_is_core()check found it withlocality=Importedand disabled batched execution for any user with Elementary installed who hadn't setrequire_batched_execution_for_custom_microbatch_strategy: True— even if they weren't using the compiled code feature at all.Our e2e tests missed this because we globally set
require_batched_execution_for_custom_microbatch_strategy: Truein the integration testdbt_project.yml, which bypasses the_microbatch_macro_is_core()gate entirely.Fix
Rename the package-level macro from
get_incremental_microbatch_sqltocapture_microbatch_compiled_code_sql. This means:get_incremental_microbatch_sqlat the package level_microbatch_macro_is_core()no longer finds Elementary's macro for users who haven't opted in → microbatch works normallyget_incremental_microbatch_sql) that callselementary.capture_microbatch_compiled_code_sql— same behavior, no functional regressionChanges
macros/edr/dbt_artifacts/microbatch/capture_microbatch_compiled_code.sql: rename macrointegration_tests/tests/test_dbt_artifacts/test_microbatch_compiled_code.py: update call to new macro nameBreaking change
Users who already set up the compiled code feature need to update their root project macro to call
elementary.capture_microbatch_compiled_code_sqlinstead ofelementary.get_incremental_microbatch_sql. This feature is very new so we expect minimal impact. Docs PR: elementary-data/elementary (docs branch).Summary by CodeRabbit
Refactor
Bug Fixes
compiled_codepopulation in the “with override” scenario versus the “without override” scenario.Documentation