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

Ct 2599/childrens parents depth #8379

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kentkr
Copy link
Contributor

@kentkr kentkr commented Aug 12, 2023

resolves #7703

Problem

This feature adds the ability to specify the maximum childs parent depth like 1@model. It functions similarly to 1+model.

Solution

A more thorough discussion can be found in #7703. The solution was to add to existing functionality like spec parsing and child/parent depth restrictions when creating the DAG.

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

@kentkr kentkr requested a review from a team as a code owner August 12, 2023 05:45
@cla-bot cla-bot bot added the cla:yes label Aug 12, 2023
@kentkr
Copy link
Contributor Author

kentkr commented Aug 12, 2023

This commit is a bit premature because some of the requested functionality in #7703 doesn't quite work.

  • 1@changed_model+1 (equivalent as1@changed_model)
    all children, and their first-order parents

Using both @ and + at the same time will fail because of the below code block.

https://github.com/kentkr/dbt-core/blob/b7aee3f5a428aa69241839196289eeed166e24f1/core/dbt/cli/flags.py#L45C1-L52C15

That means we can't specify the depth of the children (denoted with +) and the depth of the parents of those children (denoted with @). I don't know why these are incompatible but there must be a reason out there. Before I go ahead and change it I wanted to get confirmation on that. @jtcohen6 any thoughts?

@dataders
Copy link
Contributor

@kentkr hey do you mind resolving these merge conflicts so that we can take a look?

@graciegoheen graciegoheen added awaiting_response ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Sep 26, 2023
@kentkr
Copy link
Contributor Author

kentkr commented Sep 30, 2023

Hey! Yep thanks for getting back to me. I fixed the merge conflicts.

@codecov
Copy link

codecov bot commented Sep 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.39%. Comparing base (6b5db17) to head (f169b37).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8379      +/-   ##
==========================================
- Coverage   89.17%   86.39%   -2.78%     
==========================================
  Files         183      183              
  Lines       23491    23553      +62     
==========================================
- Hits        20947    20348     -599     
- Misses       2544     3205     +661     
Flag Coverage Δ
integration 86.39% <100.00%> (-0.07%) ⬇️
unit ?

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

Components Coverage Δ
Unit Tests 86.39% <100.00%> (-2.78%) ⬇️
Integration Tests 86.39% <100.00%> (-0.07%) ⬇️

Copy link
Contributor

This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days.

@github-actions github-actions bot added stale Issues that have gone stale and removed stale Issues that have gone stale labels Dec 30, 2023
@dbeatty10 dbeatty10 added the community This PR is from a community member label Mar 22, 2024
@kentkr
Copy link
Contributor Author

kentkr commented Oct 31, 2024

@dbeatty10 thanks for jumping in on this! Let me know if you need anything else from me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting_response cla:yes community This PR is from a community member ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2599] [Feature] childrens_parents depth
6 participants