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

update Number class to handle integer values #8306

Merged
merged 8 commits into from
Aug 18, 2023
Merged

Conversation

dave-connors-3
Copy link
Contributor

resolves #8153

Problem

Agate tables, which power dbt show, and therefore the dbt cloud IDE, show integer values as decimals, leading to user confusion when columns seem to have decimals when they don't! this overrides that behavior in the dbt-core customization of the agate Number class

Solution

Ensure that the Number.cast() and Number.jsonify() methods properly handle integer values.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@dave-connors-3 dave-connors-3 requested a review from a team as a code owner August 3, 2023 14:33
@cla-bot cla-bot bot added the cla:yes label Aug 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #8306 (1644628) into main (991618d) will increase coverage by 0.07%.
Report is 31 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #8306      +/-   ##
==========================================
+ Coverage   86.23%   86.30%   +0.07%     
==========================================
  Files         174      174              
  Lines       25518    25575      +57     
==========================================
+ Hits        22005    22073      +68     
+ Misses       3513     3502      -11     
Flag Coverage Δ
integration 83.08% <100.00%> (?)
unit 65.16% <85.71%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
core/dbt/clients/agate_helper.py 97.54% <100.00%> (+1.01%) ⬆️

... and 21 files with indirect coverage changes

@jtcohen6
Copy link
Contributor

jtcohen6 commented Aug 6, 2023

Thanks for putting up the PR @dave-connors-3!

The CI failure reminded me that my recommendation was incomplete.

We need to choose whether we want to cast to int sooner or later:

  • Later: just during JSON-ification
  • Sooner: as in this approach, but taken one step further to a custom Integer type

I left a comment on the issue with two potential approaches: #8153 (comment)

@emmyoop emmyoop requested review from emmyoop and removed request for ChenyuLInx and aranke August 16, 2023 18:40
@emmyoop
Copy link
Member

emmyoop commented Aug 16, 2023

@dave-connors-3 I have capacity this sprint to help get this over this finish line! I agree with @jtcohen6's recommendation to add a new Integer data type, as laid out in his comment. It's the right solution, even with the risk of being a breaking change. Let me know if you'd like some help or even if you don't have capacity right now and I can help get this in.

@dave-connors-3
Copy link
Contributor Author

thanks @emmyoop!! just pushed jerco's updates -- will test locally shortly, but would love your eyes on it whenever you have a chance!

@dave-connors-3
Copy link
Contributor Author

@emmyoop -- just got the test_agate_helper.py` tests to pass locally! I am wondering if the other tests should have been updated to assert that they were instances of the custom Integer class as well, but updating them caused local failures. Let me know what you think!

@emmyoop
Copy link
Member

emmyoop commented Aug 18, 2023

@dave-connors-3 your instincts are right that we should change the other checks as well! It's currently failing when you change them to check if they're an instance of agate_helper.Integer because the tables are being generated with agate.Table. The tables that generates is not aware of our custom types. We should update the tests to use agate_helper.table_from_rows instead as that's what we're using elsewhere in the code to account for our custom types. When you make that change, you'll be able to then check for the correct type.

So generally

t1 = agate.Table([(1, "a", None), (2, "b", None)], ("a", "b", "c"))
assert isinstance(result.column_types[0], agate.data_types.Number)

can become

t1 = agate_helper.table_from_rows([(1, "a", None), (2, "b", None)], ("a", "b", "c"))
assert isinstance(result.column_types[0], agate_helper.Integer)

result = agate_helper.merge_tables([t1, t2])
self.assertEqual(result.column_names, ("a", "b", "c"))
assert isinstance(result.column_types[0], agate.data_types.Number)
assert isinstance(result.column_types[0], agate_helper.Integer)
assert isinstance(result.column_types[1], agate.data_types.Text)
assert isinstance(result.column_types[2], agate.data_types.Number)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emmyoop this one wanted to stay a Number -- guessing it has something to do with the fact that the column is null, but wasn't positive!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pun not intended -- I wasn't positive, not the number lol

@dave-connors-3
Copy link
Contributor Author

dave-connors-3 commented Aug 18, 2023

i also seem to be cursed by the codecoverage bots nvm they like me now

Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks so much for getting this in @dave-connors-3!

.changes/unreleased/Fixes-20230803-093502.yaml Outdated Show resolved Hide resolved
@emmyoop emmyoop merged commit 661623f into main Aug 18, 2023
50 checks passed
@emmyoop emmyoop deleted the update-agate-for-int branch August 18, 2023 19:01
github-actions bot pushed a commit that referenced this pull request Aug 18, 2023
* add show test for json data

* oh changie my changie

* revert unecessary cahnge to fixture

* keep decimal class for precision methods, but return __int__ value

* jerco updates

* update integer type

* update other tests

* Update .changes/unreleased/Fixes-20230803-093502.yaml

---------

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>
(cherry picked from commit 661623f)
emmyoop pushed a commit that referenced this pull request Aug 21, 2023
* add show test for json data

* oh changie my changie

* revert unecessary cahnge to fixture

* keep decimal class for precision methods, but return __int__ value

* jerco updates

* update integer type

* update other tests

* Update .changes/unreleased/Fixes-20230803-093502.yaml

---------

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>
(cherry picked from commit 661623f)

Co-authored-by: dave-connors-3 <73915542+dave-connors-3@users.noreply.github.com>
@MichelleArk MichelleArk mentioned this pull request Aug 23, 2023
4 tasks
peterallenwebb pushed a commit that referenced this pull request Aug 30, 2023
* add show test for json data

* oh changie my changie

* revert unecessary cahnge to fixture

* keep decimal class for precision methods, but return __int__ value

* jerco updates

* update integer type

* update other tests

* Update .changes/unreleased/Fixes-20230803-093502.yaml

---------

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>
peterallenwebb added a commit that referenced this pull request Aug 30, 2023
* Add compiled node properties to run_results.json

* Include compiled-node attributes in run_results.json

* Fix typo

* Bump schema version of run_results

* Fix test assertions

* Update expected run_results to reflect new attributes

* Code review changes

* Fix mypy warnings for ManifestLoader.load() (#8443)

* revert python version for docker images (#8445)

* revert python version for docker images

* add comment to not update python version, update changelog

* Bumping version to 1.7.0b1 and generate changelog

* [CT-3013]  Fix parsing of `window_groupings` (#8454)

* Update semantic model parsing tests to check measure non_additive_dimension spec

* Make `window_groupings` default to empty list if not specified on `non_additive_dimension`

* Add changie doc for `window_groupings`  parsing fix

* update `Number` class to handle integer values (#8306)

* add show test for json data

* oh changie my changie

* revert unecessary cahnge to fixture

* keep decimal class for precision methods, but return __int__ value

* jerco updates

* update integer type

* update other tests

* Update .changes/unreleased/Fixes-20230803-093502.yaml

---------

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>

* Improve docker image README (#8212)

* Improve docker image README

- Fix unnecessary/missing newline escapes
- Remove double whitespace between parameters
- 2-space indent for extra lines in image build commands

* Add changelog entry for #8212

* ADAP-814: Refactor prep for MV updates (#8459)

* apply reformatting changes only for #8449
* add logging back to get_create_materialized_view_as_sql
* changie

* swap trigger (#8463)

* update the implementation template (#8466)

* update the implementation template

* add colon

* Split tests into classes (#8474)

* add flaky decorator

* split up tests into classes

* revert update agate for int (#8478)

* updated typing and methods to meet mypy standards (#8485)

* Convert error to conditional warning for unversioned contracted model, fix msg format (#8451)

* first pass, tests need updates

* update proto defn

* fixing tests

* more test fixes

* finish fixing test file

* reformat the message

* formatting messages

* changelog

* add event to unit test

* feedback on message structure

* WIP

* fix up event to take in all fields

* fix test

* Fix ambiguous reference error for duplicate model names across packages with tests (#8488)

* Safely remove external nodes from manifest (#8495)

* [CT-2840] Improved semantic layer protocol satisfaction tests (#8456)

* Test `SemanticModel` satisfies protocol when none of it's `Optionals` are specified

* Add tests ensuring SourceFileMetadata and FileSlice satisfiy DSI protocols

* Add test asserting Defaults obj satisfies protocol

* Add test asserting SemanticModel with optionals specified satisfies protocol

* Split dimension protocol satisfaction tests into with and without optionals

* Simplify DSI Protocol import strategy in protocol satisfaction tests

* Add test asserting DimensionValidtyParams satisfies protocol

* Add test asserting DimensionTypeParams satisfies protocol

* Split entity protocol satisfaction tests into with and without optionals

* Split measure protocol satisfication tests and add measure aggregation params satisficaition test

* Split metric protocol satisfaction test into optional specified an unspecified

Additionally, create where_filter pytest fixture

* Improve protocol satisfaction tests for MetricTypeParams and sub protocols

Specifically we added/improved protocol satisfaction tests for
- MetricTypeParams
- MetricInput
- MetricInputMeasure
- MetricTimeWindow

* Convert to using mashumaro jsonschema with acceptable performance (#8437)

* Regenerate run_results schema after merging in changes from main.

---------

Co-authored-by: Gerda Shank <gerda@dbtlabs.com>
Co-authored-by: Matthew McKnight <91097623+McKnight-42@users.noreply.github.com>
Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com>
Co-authored-by: Quigley Malcolm <QMalcolm@users.noreply.github.com>
Co-authored-by: dave-connors-3 <73915542+dave-connors-3@users.noreply.github.com>
Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>
Co-authored-by: Jaime Martínez Rincón <jaime@jamezrin.name>
Co-authored-by: Mike Alfare <13974384+mikealfare@users.noreply.github.com>
Co-authored-by: Michelle Ark <MichelleArk@users.noreply.github.com>
emmyoop added a commit that referenced this pull request Sep 6, 2023
* add show test for json data

* oh changie my changie

* revert unecessary cahnge to fixture

* keep decimal class for precision methods, but return __int__ value

* jerco updates

* update integer type

* update other tests

* Update .changes/unreleased/Fixes-20230803-093502.yaml

---------

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>
emmyoop added a commit that referenced this pull request Sep 7, 2023
* update `Number` class to handle integer values (#8306)

* add show test for json data

* oh changie my changie

* revert unecessary cahnge to fixture

* keep decimal class for precision methods, but return __int__ value

* jerco updates

* update integer type

* update other tests

* Update .changes/unreleased/Fixes-20230803-093502.yaml

---------

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>

* account for integer vs number on table merges

* add tests for combining number with integer.

* add unit test when nulls are added

* cant none as an Integer

* fix null tests

---------

Co-authored-by: dave-connors-3 <73915542+dave-connors-3@users.noreply.github.com>
Co-authored-by: Dave Connors <dave.connors@fishtownanalytics.com>
github-actions bot pushed a commit that referenced this pull request Sep 7, 2023
* update `Number` class to handle integer values (#8306)

* add show test for json data

* oh changie my changie

* revert unecessary cahnge to fixture

* keep decimal class for precision methods, but return __int__ value

* jerco updates

* update integer type

* update other tests

* Update .changes/unreleased/Fixes-20230803-093502.yaml

---------

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>

* account for integer vs number on table merges

* add tests for combining number with integer.

* add unit test when nulls are added

* cant none as an Integer

* fix null tests

---------

Co-authored-by: dave-connors-3 <73915542+dave-connors-3@users.noreply.github.com>
Co-authored-by: Dave Connors <dave.connors@fishtownanalytics.com>
(cherry picked from commit be94bf1)
emmyoop added a commit that referenced this pull request Sep 7, 2023
* update `Number` class to handle integer values (#8306)

* add show test for json data

* oh changie my changie

* revert unecessary cahnge to fixture

* keep decimal class for precision methods, but return __int__ value

* jerco updates

* update integer type

* update other tests

* Update .changes/unreleased/Fixes-20230803-093502.yaml

---------

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>

* account for integer vs number on table merges

* add tests for combining number with integer.

* add unit test when nulls are added

* cant none as an Integer

* fix null tests

---------

Co-authored-by: dave-connors-3 <73915542+dave-connors-3@users.noreply.github.com>
Co-authored-by: Dave Connors <dave.connors@fishtownanalytics.com>
(cherry picked from commit be94bf1)

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>
emmyoop added a commit that referenced this pull request Sep 11, 2023
* update `Number` class to handle integer values (#8306)

* add show test for json data

* oh changie my changie

* revert unecessary cahnge to fixture

* keep decimal class for precision methods, but return __int__ value

* jerco updates

* update integer type

* update other tests

* Update .changes/unreleased/Fixes-20230803-093502.yaml

---------

Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>

* account for integer vs number on table merges

* add tests for combining number with integer.

* add unit test when nulls are added

* cant none as an Integer

* fix null tests

---------

Co-authored-by: dave-connors-3 <73915542+dave-connors-3@users.noreply.github.com>
Co-authored-by: Dave Connors <dave.connors@fishtownanalytics.com>
colin-rogers-dbt pushed a commit that referenced this pull request Sep 11, 2023
* update `Number` class to handle integer values (#8306)

* add show test for json data

* oh changie my changie

* revert unecessary cahnge to fixture

* keep decimal class for precision methods, but return __int__ value

* jerco updates

* update integer type

* update other tests

* Update .changes/unreleased/Fixes-20230803-093502.yaml

---------



* account for integer vs number on table merges

* add tests for combining number with integer.

* add unit test when nulls are added

* cant none as an Integer

* fix null tests

---------

Co-authored-by: dave-connors-3 <73915542+dave-connors-3@users.noreply.github.com>
Co-authored-by: Dave Connors <dave.connors@fishtownanalytics.com>
@aranke aranke mentioned this pull request Jul 12, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2848] [Bug] dbt show adding decimal places to non-decimal values
3 participants