Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add dbt_valid_to_current #6308
base: current
Are you sure you want to change the base?
add dbt_valid_to_current #6308
Changes from 17 commits
a2d5c4c
3d54fee
2093b8b
e57fbc2
d1d9aeb
614e368
7b846e7
99cfb62
0d788e4
44b6e14
7729456
50cdb0a
1db6875
82864c0
bea946d
33483f0
9be457a
40eac92
bf40b21
c1eb3f1
15e71c9
3dd2a3d
704879f
3fb5435
50c0f35
edd6df8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "simplifies your SQL queries by avoiding
NULL
checks" mean?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i meant users not needing to include checks or add'l logic to check for null records.
i can remove if it's confusing or clarify further!
Check warning on line 218 in website/docs/docs/build/snapshots.md
GitHub Actions / vale
[vale] website/docs/docs/build/snapshots.md#L218
Raw output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're missing some clarity that this is set to
NULL
(or whatever you have setdbt_valid_to_current
to for current records)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, i would rephrase this as suggested above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, rephrase as suggested above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know where we want this, but we should probably have a migration callout/warning like what we have for the other new configs cc: @dbeatty10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, rephrase as suggested above
thinking about this more, i do like that we're calling out that the main use case for this is "a future date"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"date date"
Check warning on line 55 in website/docs/reference/resource-configs/dbt_valid_to_current.md
GitHub Actions / vale
[vale] website/docs/reference/resource-configs/dbt_valid_to_current.md#L55
Raw output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a more specific descriptor here for the heading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah here's the migration callout - I think we just want to make sure this warning exists elsewhere as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"rebuild your snapshot table" would be very risky (lose all of your historical data!)
Check warning on line 101 in website/docs/reference/resource-configs/dbt_valid_to_current.md
GitHub Actions / vale
[vale] website/docs/reference/resource-configs/dbt_valid_to_current.md#L101
Raw output
Check warning on line 102 in website/docs/reference/resource-configs/dbt_valid_to_current.md
GitHub Actions / vale
[vale] website/docs/reference/resource-configs/dbt_valid_to_current.md#L102
Raw output