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.

Sign up for GitHub

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

implement type_boolean macro #5875

Merged
merged 2 commits into from
Sep 21, 2022
Merged

implement type_boolean macro #5875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5df41fd
implement type_boolean macro
jpmmcneill Sep 18, 2022
2bb86b5
changie result
jpmmcneill Sep 19, 2022
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
7 changes: 7 additions & 0 deletions .changes/unreleased/Features-20220919-231414.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: Features
body: add type_boolean as a data type macro
time: 2022-09-19T23:14:14.9871+01:00
custom:
Author: jpmmcneill
Issue: "5739"
PR: "5875"
1 change: 1 addition & 0 deletions core/dbt/adapters/base/column.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class Column:
"TIMESTAMP": "TIMESTAMP",
"FLOAT": "FLOAT",
"INTEGER": "INT",
"BOOLEAN": "BOOLEAN",
}
column: str
dtype: str
Expand Down
14 changes: 13 additions & 1 deletion core/dbt/include/global_project/macros/utils/data_types.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ The TIMESTAMP_* variation associated with TIMESTAMP is specified by the TIMESTAM
{{ return(api.Column.translate_type("float")) }}
{% endmacro %}

{# numeric ------------------------------------------------ #}
{# numeric ------------------------------------------------- #}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

every other comment had this many dashes 🙃


{%- macro type_numeric() -%}
{{ return(adapter.dispatch('type_numeric', 'dbt')()) }}
Expand Down Expand Up @@ -115,3 +115,15 @@ the precision and scale explicitly.)

-- returns 'int' everywhere, except BigQuery, where it returns 'int64'
-- (but BigQuery also now accepts 'int' as a valid alias for 'int64')

{# bool ------------------------------------------------- #}

{%- macro type_boolean() -%}
{{ return(adapter.dispatch('type_boolean', 'dbt')()) }}
{%- endmacro -%}

{%- macro default__type_boolean() -%}
{{ return(api.Column.translate_type("boolean")) }}
{%- endmacro -%}

-- returns 'boolean' everywhere. BigQuery accepts 'boolean' as a valid alias for 'bool'
24 changes: 24 additions & 0 deletions tests/adapter/dbt/tests/adapter/utils/data_types/test_type_boolean.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import pytest
from dbt.tests.adapter.utils.data_types.base_data_type_macro import BaseDataTypeMacro

seeds__expected_csv = """boolean_col
True
""".lstrip()

models__actual_sql = """
select cast('True' as {{ type_boolean() }}) as boolean_col
"""
Comment on lines +4 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to cover both boolean options at the very least.

Suggested change
seeds__expected_csv = """boolean_col
True
""".lstrip()
models__actual_sql = """
select cast('True' as {{ type_boolean() }}) as boolean_col
"""
seeds__expected_csv = """boolean_col
True
False
""".lstrip()
models__actual_sql = """
select cast('True' as {{ type_boolean() }}) as boolean_col
union all
select cast('False' as {{ type_boolean() }}) as boolean_col
"""

Even better

But test cases covering the truth tables for conjunction, disjunction, and negation would be even better. That way, we are verifying that everything is acting like booleans.

image image image

Something like the following untested code:

seeds__boolean_permutations_csv = """
x,y
False,False
True,False
False,True
True,True
""".lstrip()

seeds__expected_csv = """
x,y,conjunction,disjunction,negation_x
False,False,False,False,True
True,False,False,True,False
False,True,False,True,True
True,True,True,True,False
""".lstrip()

models__actual_sql = """
select
    x,
    y,
    x and y as conjunction,
    x or y as disjunction,
    not x as negation_x
from {{ ref("boolean_permutations" }}

Best? 🤷

Even though BOOLEAN is a data type described in the SQL standard, some databases don't have it (looking at you, SQL Server!).

If we are feeling extra magnanimous, we could change all the True/False values in the seeds to be 1/0 instead.

I'm hoping it wouldn't be necessary, but we could update the models__actual_sql definition so that x/y values are replaced with the following instead:

  • cast(x as {{ type_boolean() }})
  • cast(y as {{ type_boolean() }})

Copy link
Contributor

@dbeatty10 dbeatty10 Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Update:

The "Even better" example I gave above didn't actually test the type_boolean() macro at all! 😅

Something like this should fix that situation:

models__actual_sql = """
select
    cast(x as {{ type_boolean() }}) as x_bool,
    cast(y as {{ type_boolean() }}) as y_bool,
    cast(x as {{ type_boolean() }}) and cast(y as {{ type_boolean() }}) as conjunction,
    cast(x as {{ type_boolean() }}) or cast(y as {{ type_boolean() }}) as disjunction,
    not cast(x as {{ type_boolean() }}) as negation_x
from {{ ref("boolean_permutations" }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cheers @dbeatty10. I'll take a second pass at this later today :)



class BaseTypeBoolean(BaseDataTypeMacro):
@pytest.fixture(scope="class")
def seeds(self):
return {"expected.csv": seeds__expected_csv}

@pytest.fixture(scope="class")
def models(self):
return {"actual.sql": self.interpolate_macro_namespace(models__actual_sql, "type_boolean")}


class TestTypeBoolean(BaseTypeBoolean):
pass