Fix Execute SQL target #1685
Fix Execute SQL target #1685
Conversation
WalkthroughThe change updates the internal implementation of the Changes
Sequence Diagram(s)sequenceDiagram
participant JS as DBTCoreProjectIntegration (TypeScript)
participant Py as Python DbtProject
JS->>Py: Instantiate DbtProject(project_dir, profiles_dir, target_path, defer_to_prod, manifest_path, favor_state, target_name)
Py-->>JS: Returns DbtProject instance with target_name set
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…on-with-target-switch
There was a problem hiding this comment.
Pull Request Overview
This PR addresses the issue in executing SQL by ensuring the Python project uses the selected target when creating a DbtProject instance.
- Fixes the DbtProject constructor call to include the proper target_name parameter.
- Updates the code in DBTCoreProjectIntegration to use this.targetName when invoking createPythonDbtProject().
Comments suppressed due to low confidence (1)
src/dbt_client/dbtCoreIntegration.ts:473
- Verify that the DbtProject constructor accepts 'target_name' as a parameter and that its positional order aligns with the intended API design. Consider adding inline documentation if the parameter order is significant.
await bridge.ex`project = DbtProject(target_name=${this.targetName}, project_dir=${this.projectRoot.fsPath}, profiles_dir=${this.profilesDir}, target_path=${targetPath}, defer_to_prod=${deferToProduction}, manifest_path=${manifestPath}, favor_state=${favorState}) if 'project' not in locals() else project`
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 69eeccd in 1 minute and 23 seconds. Click for details.
- Reviewed
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/dbt_client/dbtCoreIntegration.ts:473
- Draft comment:
Good change – you now pass the selected target (target_name) when instantiating DbtProject, ensuring SQL execution uses the chosen target. However, ensure that this.targetName is set (i.e., not undefined) before calling createPythonDbtProject. If it might be undefined, consider adding a check or fallback to avoid potential issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is about a real code change - adding target_name parameter. However, the suggestion to validate this.targetName is speculative since we don't know if undefined is a valid value for the Python DbtProject constructor. The comment is asking for confirmation/verification rather than pointing out a clear issue. The code may work fine with undefined being passed through. I could be wrong about whether undefined would cause issues in the Python bridge layer. Perhaps there's a real risk of errors if undefined is passed through. Even if undefined could cause issues, the comment is still phrased as a verification request rather than pointing out a clear problem. If undefined was definitely problematic, the comment should directly state that and suggest a specific fix. The comment should be deleted because it is speculative and asks for verification rather than identifying a clear issue that needs to be fixed.
Workflow ID: wflow_D3rPHobRFo5iHOoB
You can customize 
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/dbt_client/dbtCoreIntegration.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest)
- GitHub Check: test (windows-latest)
src/dbt_client/dbtCoreIntegration.ts
Outdated
| await bridge.ex`project = DbtProject(target_name=${ | ||
| this.targetName | ||
| }, project_dir=${this.projectRoot.fsPath}, profiles_dir=${this.profilesDir}, target_path=${targetPath}, defer_to_prod=${deferToProduction}, manifest_path=${manifestPath}, favor_state=${favorState}) if 'project' not in locals() else project`; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix potential undefined target_name during initialization.
The addition of target_name parameter correctly addresses the PR objective of using the selected target for SQL execution. However, there's a critical issue: this.targetName can be undefined during the initial call to createPythonDbtProject() in initializeProject() (line 503), since refreshProjectConfig() (which sets this.targetName) hasn't been called yet.
Apply this diff to handle the undefined case:
- await bridge.ex`project = DbtProject(target_name=${
- this.targetName
- }, project_dir=${this.projectRoot.fsPath}, profiles_dir=${this.profilesDir}, target_path=${targetPath}, defer_to_prod=${deferToProduction}, manifest_path=${manifestPath}, favor_state=${favorState}) if 'project' not in locals() else project`;
+ await bridge.ex`project = DbtProject(target_name=${
+ this.targetName || null
+ }, project_dir=${this.projectRoot.fsPath}, profiles_dir=${this.profilesDir}, target_path=${targetPath}, defer_to_prod=${deferToProduction}, manifest_path=${manifestPath}, favor_state=${favorState}) if 'project' not in locals() else project`;Verify how the Python DbtProject constructor handles null target_name:
🏁 Script executed:
#!/bin/bash
# Description: Find the Python DbtProject constructor to verify parameter handling
# Search for DbtProject class definition and constructor
ast-grep --pattern $'class DbtProject:
$$$
def __init__($$$):
$$$'
# Also search for target_name parameter usage
rg -A 10 -B 5 "target_name.*=" --type pyLength of output: 39378
Guard against undefined targetName in createPythonDbtProject.
To prevent passing undefined (which the Python bridge may not handle) into the DbtProject constructor—which expects an Optional[str]—we should fall back to null so Python receives None.
Locations to update:
- File:
src/dbt_client/dbtCoreIntegration.ts - Method:
createPythonDbtProject() - Around lines 473–475
Suggested diff:
- await bridge.ex`project = DbtProject(target_name=${
- this.targetName
- }, project_dir=${this.projectRoot.fsPath}, profiles_dir=${this.profilesDir}, target_path=${targetPath}, defer_to_prod=${deferToProduction}, manifest_path=${manifestPath}, favor_state=${favorState}) if 'project' not in locals() else project`;
+ await bridge.ex`project = DbtProject(target_name=${
+ this.targetName ?? null
+ }, project_dir=${this.projectRoot.fsPath}, profiles_dir=${this.profilesDir}, target_path=${targetPath}, defer_to_prod=${deferToProduction}, manifest_path=${manifestPath}, favor_state=${favorState}) if 'project' not in locals() else project`;This uses the nullish coalescing operator (??) to ensure that when this.targetName is undefined, the Python side sees None.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await bridge.ex`project = DbtProject(target_name=${ | |
| this.targetName | |
| }, project_dir=${this.projectRoot.fsPath}, profiles_dir=${this.profilesDir}, target_path=${targetPath}, defer_to_prod=${deferToProduction}, manifest_path=${manifestPath}, favor_state=${favorState}) if 'project' not in locals() else project`; | |
| await bridge.ex`project = DbtProject(target_name=${ | |
| this.targetName ?? null | |
| }, project_dir=${this.projectRoot.fsPath}, profiles_dir=${this.profilesDir}, target_path=${targetPath}, defer_to_prod=${deferToProduction}, manifest_path=${manifestPath}, favor_state=${favorState}) if 'project' not in locals() else project`; |
🤖 Prompt for AI Agents
In src/dbt_client/dbtCoreIntegration.ts around lines 473 to 475, the code passes
this.targetName directly to the Python DbtProject constructor, but
this.targetName can be undefined during initial calls. To fix this, update the
code to use the nullish coalescing operator (??) to pass null instead of
undefined when this.targetName is not set, ensuring the Python side receives
None and preventing errors.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 12c4ef5 in 1 minute and 0 seconds. Click for details.
- Reviewed
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/dbt_client/dbtCoreIntegration.ts:470
- Draft comment:
The revised template literal now inlines the interpolation fortarget_name(i.e. ${this.targetName}) without a newline, ensuring the correct target is passed to the DbtProject constructor. This concise formatting helps avoid unintended whitespace which might have caused the Execute SQL target issue. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining the change made in the code. It doesn't provide any actionable feedback or suggestions for improvement. It doesn't align with the rules for useful comments.
Workflow ID: wflow_uQgqpeH4n96jDdaJ
You can customize
|
Tested this change but this is a no-op. This line is only executed once at init time: Instantiates a DbtProject object with the given parameters if project is not already defined in the local scope. Otherwise, it reuses the existing project object (avoiding re-instantiation). |
Summary
fix for #1676
Testing
npm testImportant
Fixes bug in
createPythonDbtProject()to ensure correct target is used when executing SQL indbtCoreIntegration.ts.dbtCoreIntegration.ts,createPythonDbtProject()now includestarget_nameparameter to ensure correct target is used when executing SQL.npm test.This description was created by
for 12c4ef5. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit