Update core to support DSI 0.8.3 #10990

WilliamDee · 2024-11-12T21:36:39Z

Problem

Upgrade to DSI 0.8.3 to support passing custom time grain in the yaml spec

schema change PR: dbt-labs/schemas.getdbt.com#70

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 or 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.
This PR includes type annotations for new and modified functions.

github-actions · 2024-11-12T21:36:56Z

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 · 2024-11-12T21:44:28Z

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.13%. Comparing base (fdfe03d) to head (363e44b).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10990      +/-   ##
==========================================
- Coverage   89.17%   89.13%   -0.05%     
==========================================
  Files         183      183              
  Lines       23783    23784       +1     
==========================================
- Hits        21209    21200       -9     
- Misses       2574     2584      +10

Flag	Coverage Δ
integration	`86.44% <86.95%> (-0.13%)`	⬇️
unit	`62.20% <86.95%> (+0.03%)`	⬆️

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

Components	Coverage Δ
Unit Tests	`62.20% <86.95%> (+0.03%)`	⬆️
Integration Tests	`86.44% <86.95%> (-0.13%)`	⬇️

QMalcolm

This looks good! I had one question about MFs/DSIs expectations on case. Depending on the answer to that, we might need to make changes, or we might just be good to go 🙂

core/dbt/artifacts/resources/v1/semantic_model.py

core/dbt/artifacts/resources/v1/metric.py

QMalcolm · 2024-11-14T20:10:16Z

core/dbt/parser/schema_yaml_readers.py

    def _get_optional_time_window(
        self, unparsed_window: Optional[str]
    ) -> Optional[MetricTimeWindow]:
        if unparsed_window is not None:
-            parts = unparsed_window.split(" ")
+            parts = unparsed_window.lower().split(" ")


Does DSI/Metricflow require these be lowercase now? If so, this may be breaking as for already parsed manifests 🤔

Nope! It should not be case sensitive. This is lowercasing it before we get it so that the user doesn't need to worry about lowercasing it but we can expect lowercase on our side. Will added tests for case sensitivity in DSI, too.

The words that concern me are

but we can expect lowercase on our side

I'm not sure you can unfortunately, but I could be wrong about that. Could you point me to the tests around case sensitivity in DSI?

Yep!
https://github.com/dbt-labs/dbt-semantic-interfaces/blob/f3a50f02b962ce7193b801784740e244057ddb69/tests/parsing/test_metric_parsing.py#L215-L216
and
https://github.com/dbt-labs/dbt-semantic-interfaces/blob/f3a50f02b962ce7193b801784740e244057ddb69/tests/parsing/test_metric_parsing.py#L240-L241

Let me go back and make sure all other fields have this covered, too

Alright @QMalcolm - sorry for the delay on this, had some other high prios and then didn't want to mess with code freeze! 🥶

We bumped the version of DSI one more time with some changes to fix the issues with case sensitivity. This is the PR with those changes, but these commits are most relevant: 1, 2.
Let us know if you think there are any other issues here! The thorough review was much appreciated 🙏
Once approved, we shouldn't merge quite yet - just because we want to coordinate the merge timing so that we will have time to update mantle between when this merges and the next sync to mantle.

cc @WilliamDee

I think github screwed up those links somehow. They point to https://github.com/dbt-labs/dbt-core/pull/2083a249889252823b097098cb03471f2db0d00e and https://github.com/dbt-labs/dbt-core/pull/00abab69a8b4537abcc0ea9e544547e99a3c27a2. Which are commits, but on DSI, not core.

For the public observer, the correct links are

dbt-labs/dbt-semantic-interfaces@2083a24

dbt-labs/dbt-semantic-interfaces@00abab6

QMalcolm

Looks good to me! Thank you for resolving the case sensitivity issue ❤️

…window, so removing tests that includes them otherwise

WilliamDee requested review from a team as code owners November 12, 2024 21:36

WilliamDee requested review from wpowers-dbt and removed request for a team November 12, 2024 21:36

cla-bot bot added the cla:yes label Nov 12, 2024

github-actions bot added the community This PR is from a community member label Nov 12, 2024

WilliamDee force-pushed the will/update-dsi branch from 5c443d7 to 890a1b0 Compare November 12, 2024 23:45

WilliamDee changed the title ~~Update to DSI 0.8.0~~ Update to DSI 0.8.1 Nov 12, 2024

WilliamDee force-pushed the will/update-dsi branch from 890a1b0 to 0256834 Compare November 12, 2024 23:47

WilliamDee changed the title ~~Update to DSI 0.8.1~~ Update core to support DSI 0.8.1 Nov 12, 2024

WilliamDee requested a review from courtneyholcomb November 12, 2024 23:49

QMalcolm reviewed Nov 14, 2024

View reviewed changes

QMalcolm added the artifact_minor_upgrade To bypass the CI check by confirming that the change is not breaking label Nov 14, 2024

courtneyholcomb changed the title ~~Update core to support DSI 0.8.1~~ Update core to support DSI 0.8.2 Dec 2, 2024

courtneyholcomb requested a review from QMalcolm December 2, 2024 22:47

courtneyholcomb changed the title ~~Update core to support DSI 0.8.2~~ Update core to support DSI 0.8.3 Dec 3, 2024

QMalcolm approved these changes Dec 3, 2024

View reviewed changes

WilliamDee and others added 8 commits December 4, 2024 11:57

Upgrade DSI to 0.8.1 and fix class changes

57fd0fa

fix tests

5879461

add more tests

66fd87d

changelog

6fe69a0

Bump DSI to 0.8.2

7bf447a

Bump DSI to 0.8.3

074375b

Update changelog

7aae48d

Custom grain was decided to only be supported in input_metric.offset_…

363e44b

…window, so removing tests that includes them otherwise

WilliamDee force-pushed the will/update-dsi branch from e0fcb99 to 363e44b Compare December 4, 2024 16:57

ChenyuLInx merged commit ff6745c into main Dec 5, 2024
54 of 56 checks passed

ChenyuLInx deleted the will/update-dsi branch December 5, 2024 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update core to support DSI 0.8.3 #10990

Update core to support DSI 0.8.3 #10990

WilliamDee commented Nov 12, 2024 •

edited by courtneyholcomb

Loading

github-actions bot commented Nov 12, 2024

codecov bot commented Nov 12, 2024 •

edited

Loading

QMalcolm left a comment

QMalcolm Nov 14, 2024 •

edited

Loading

courtneyholcomb Nov 15, 2024

QMalcolm Nov 15, 2024 •

edited

Loading

courtneyholcomb Nov 15, 2024

courtneyholcomb Nov 15, 2024

courtneyholcomb Dec 2, 2024

QMalcolm Dec 3, 2024

QMalcolm Dec 3, 2024

QMalcolm left a comment

Update core to support DSI 0.8.3 #10990

Update core to support DSI 0.8.3 #10990

Conversation

WilliamDee commented Nov 12, 2024 • edited by courtneyholcomb Loading

Problem

Solution

Checklist

github-actions bot commented Nov 12, 2024

codecov bot commented Nov 12, 2024 • edited Loading

Codecov Report

QMalcolm left a comment

Choose a reason for hiding this comment

QMalcolm Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

courtneyholcomb Nov 15, 2024

Choose a reason for hiding this comment

QMalcolm Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

courtneyholcomb Nov 15, 2024

Choose a reason for hiding this comment

courtneyholcomb Nov 15, 2024

Choose a reason for hiding this comment

courtneyholcomb Dec 2, 2024

Choose a reason for hiding this comment

QMalcolm Dec 3, 2024

Choose a reason for hiding this comment

QMalcolm Dec 3, 2024

Choose a reason for hiding this comment

QMalcolm left a comment

Choose a reason for hiding this comment

WilliamDee commented Nov 12, 2024 •

edited by courtneyholcomb

Loading

codecov bot commented Nov 12, 2024 •

edited

Loading

QMalcolm Nov 14, 2024 •

edited

Loading

QMalcolm Nov 15, 2024 •

edited

Loading