Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues with inline nodes and selectors #10264

Merged
merged 8 commits into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Fixes-20240605-111652.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Fixes
body: Fix issues with selectors and inline nodes
time: 2024-06-05T11:16:52.187667-04:00
custom:
Author: gshank
Issue: 8943 9269
8 changes: 7 additions & 1 deletion core/dbt/cli/flags.py
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,10 @@ def _assign_params(
params_assigned_from_default, ["WARN_ERROR", "WARN_ERROR_OPTIONS"]
)

# Handle arguments mutually exclusive with INLINE
self._assert_mutually_exclusive(params_assigned_from_default, ["SELECT", "INLINE"])
self._assert_mutually_exclusive(params_assigned_from_default, ["SELECTOR", "INLINE"])

# Support lower cased access for legacy code.
params = set(
x for x in dir(self) if not callable(getattr(self, x)) and not x.startswith("__")
Expand All @@ -315,7 +319,9 @@ def _assert_mutually_exclusive(
"""
set_flag = None
for flag in group:
flag_set_by_user = flag.lower() not in params_assigned_from_default
flag_set_by_user = (
hasattr(self, flag) and flag.lower() not in params_assigned_from_default
)
if flag_set_by_user and set_flag:
raise DbtUsageException(
f"{flag.lower()}: not allowed with argument {set_flag.lower()}"
Expand Down
6 changes: 6 additions & 0 deletions core/dbt/task/compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,12 @@
)
sql_node = block_parser.parse_remote(self.args.inline, "inline_query")
process_node(self.config, self.manifest, sql_node)
# Special hack to remove disabled, if it's there. This would only happen
# if all models are disabled in dbt_project
Copy link
Contributor

@MichelleArk MichelleArk Jun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if all models are disabled, we effectively re-enable all of them? I think I'm missing something here as to why this would be safe to do, and how we know at this point that all models have been disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way that an inline node would be disabled is if somebody has all models to enabled: false (like in the test case) because there's no place else for enabled: false to come from. This just enables and moves the temporary inline node that was just created. It doesn't affect anything else in the manifest.

if sql_node.config.enabled is False:
sql_node.config.enabled = True
self.manifest.disabled.pop(sql_node.unique_id)
self.manifest.nodes[sql_node.unique_id] = sql_node

Check warning on line 112 in core/dbt/task/compile.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/task/compile.py#L109-L112

Added lines #L109 - L112 were not covered by tests
# keep track of the node added to the manifest
self._inline_node_id = sql_node.unique_id
except CompilationError as exc:
Expand Down
8 changes: 6 additions & 2 deletions core/dbt/task/runnable.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from datetime import datetime
from multiprocessing.dummy import Pool as ThreadPool
from pathlib import Path
from typing import AbstractSet, Dict, Iterable, List, Optional, Set, Tuple
from typing import AbstractSet, Dict, Iterable, List, Optional, Set, Tuple, Union

import dbt.exceptions
import dbt.tracking
Expand Down Expand Up @@ -108,7 +108,11 @@

def get_selection_spec(self) -> SelectionSpec:
default_selector_name = self.config.get_default_selector_name()
if self.args.selector:
spec: Union[SelectionSpec, bool]
if hasattr(self.args, "inline") and self.args.inline:

Check warning on line 112 in core/dbt/task/runnable.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/task/runnable.py#L112

Added line #L112 was not covered by tests
# We want an empty selection spec.
spec = parse_difference(None, None)
elif self.args.selector:

Check warning on line 115 in core/dbt/task/runnable.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/task/runnable.py#L114-L115

Added lines #L114 - L115 were not covered by tests
# use pre-defined selector (--selector)
spec = self.config.get_selector(self.args.selector)
elif not (self.selection_arg or self.exclusion_arg) and default_selector_name:
Expand Down
3 changes: 3 additions & 0 deletions tests/functional/dbt_runner/test_dbt_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ def test_command_mutually_exclusive_option(self, dbt: dbtRunner) -> None:
res = dbt.invoke(["deps", "--warn-error", "--warn-error-options", '{"include": "all"}'])
assert type(res.exception) == DbtUsageException

res = dbt.invoke(["compile", "--select", "models", "--inline", "select 1 as id"])
assert type(res.exception) == DbtUsageException

def test_invalid_command(self, dbt: dbtRunner) -> None:
res = dbt.invoke(["invalid-command"])
assert type(res.exception) == DbtUsageException
Expand Down
64 changes: 64 additions & 0 deletions tests/functional/graph_selection/test_inline.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import pytest

from dbt.cli.exceptions import DbtUsageException
from dbt.tests.util import run_dbt, run_dbt_and_capture, write_file

selectors_yml = """
selectors:
- name: test_selector
description: Exclude everything
default: true
definition:
method: package
value: "foo"
"""

dbt_project_yml = """
name: test
profile: test
flags:
send_anonymous_usage_stats: false
"""

dbt_project_yml_disabled_models = """
name: test
profile: test
flags:
send_anonymous_usage_stats: false
models:
+enabled: false
"""


class TestCompileInlineWithSelector:
@pytest.fixture(scope="class")
def models(self):
return {
"first_model.sql": "select 1 as id",
}

@pytest.fixture(scope="class")
def selectors(self):
return selectors_yml

def test_inline_selectors(self, project):
(results, log_output) = run_dbt_and_capture(
["compile", "--inline", "select * from {{ ref('first_model') }}"]
)
assert len(results) == 1
assert "Compiled inline node is:" in log_output

# Set all models to disabled, check that we still get inline result
write_file(dbt_project_yml_disabled_models, project.project_root, "dbt_project.yml")
(results, log_output) = run_dbt_and_capture(["compile", "--inline", "select 1 as id"])
assert len(results) == 1

# put back non-disabled dbt_project and check for mutually exclusive error message
# for --select and --inline
write_file(dbt_project_yml, project.project_root, "dbt_project.yml")
with pytest.raises(DbtUsageException):
run_dbt(["compile", "--select", "first_model", "--inline", "select 1 as id"])

# check for mutually exclusive --selector and --inline
with pytest.raises(DbtUsageException):
run_dbt(["compile", "--selector", "test_selector", "--inline", "select 1 as id"])
Loading