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

Include Compiled Node Attributes in run_results.json #8492

Merged
merged 26 commits into from
Aug 30, 2023

Conversation

peterallenwebb
Copy link
Contributor

resolves #7519

Problem

Solution

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

@cla-bot cla-bot bot added the cla:yes label Aug 24, 2023
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Patch coverage: 92.30% and project coverage change: -0.01% ⚠️

Comparison is base (f063e4e) 86.34% compared to head (bda7219) 86.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8492      +/-   ##
==========================================
- Coverage   86.34%   86.34%   -0.01%     
==========================================
  Files         174      174              
  Lines       25531    25541      +10     
==========================================
+ Hits        22046    22054       +8     
- Misses       3485     3487       +2     
Flag Coverage Δ
integration 83.14% <92.30%> (-0.01%) ⬇️
unit 65.09% <53.84%> (-0.01%) ⬇️

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

Files Changed Coverage Δ
core/dbt/tests/util.py 86.50% <85.71%> (-0.07%) ⬇️
core/dbt/contracts/results.py 97.80% <100.00%> (+0.03%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@peterallenwebb peterallenwebb marked this pull request as ready for review August 29, 2023 14:04
@peterallenwebb peterallenwebb requested review from a team as code owners August 29, 2023 14:04
@peterallenwebb peterallenwebb requested review from vadim82 and martynydbt and removed request for a team August 29, 2023 14:04
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

tests/functional/invariants/invariants.py is kind of odd naming. Why "invariants"? Why the doubled up name? Is there some reason you put "get_run_results" in that file instead of in dbt/tests/util.py along with "get_manifest"?

Looks like artifacts tests need to be fixed...

@peterallenwebb
Copy link
Contributor Author

tests/functional/invariants/invariants.py is kind of odd naming. Why "invariants"? Why the doubled up name?

@gshank I agree this was awkwardly named. The conditions I wound up calling "assertions" were initially "invariants" for nerdy reasons, but I thought calling them assertions was more friendly, and I should have made the naming consistent.

Let me know what you think of the new names I chose.

Is there some reason you put "get_run_results" in that file instead of in dbt/tests/util.py along with "get_manifest"?

Only that it didn't occur to me. I've moved the code to tests/util.py.

Looks like artifacts tests need to be fixed...

Fixed!

gshank and others added 13 commits August 30, 2023 14:49
* revert python version for docker images

* add comment to not update python version, update changelog
* 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
* 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

- 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
* apply reformatting changes only for #8449
* add logging back to get_create_materialized_view_as_sql
* changie
* update the implementation template

* add colon
* add flaky decorator

* split up tests into classes
…, 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
MichelleArk and others added 6 commits August 30, 2023 14:49
* 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
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

Looks good!

@peterallenwebb peterallenwebb merged commit fc1a14a into main Aug 30, 2023
@peterallenwebb peterallenwebb deleted the paw/copiled-code-in-run-results branch August 30, 2023 21:28
@graciegoheen graciegoheen added the user docs [docs.getdbt.com] Needs better documentation label Sep 25, 2023
@FishtownBuildBot
Copy link
Collaborator

Opened a new issue in dbt-labs/docs.getdbt.com: dbt-labs/docs.getdbt.com#4125

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes user docs [docs.getdbt.com] Needs better documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2537] Include "compiled" node attributes in run results