-
Notifications
You must be signed in to change notification settings - Fork 179
[WIP] Allow Explicit Merge actions #1018
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
base: main
Are you sure you want to change the base?
[WIP] Allow Explicit Merge actions #1018
Conversation
…hers Signed-off-by: Dmitry Volodin <[email protected]>
|
There are some extra actions on docs + changelog, but the code is ready for review. @benc-db could you please trigger the tests on your side? My functional tests with snapshots traditionally doesn't work. |
benc-db
left a comment
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.
Nice. I'll need to sleep on the name of the config, but the approach and test looks good.
|
Yep, of course, I'll write a docs entry meanwhile. |
|
For transparency - it was decided to pause this PR activities until the principal decision would be made to introduce that functionality in that form. |
|
@mi-volodin @benc-db are there any further updates on this? We have a similar need for multiple matches and I'd rather use something native rather than creating a modified version. With the native databricks merge functionality progressing very fast, it makes sense to have custom sql expressions for the merge rather than loads of potential keys and extra config options which become increasingly more complex to maintain. |
|
@asos-dipeshbhundia there were no further proceedings, IIRC mostly due to the coincidental release of dbt fusion at that time. I personally also still would like to have this functionality, but only if the concept would be approved to go on with by @benc-db . The branch is apparently outdated and I'll be happy to rebase it if needed. |
|
@mi-volodin Thanks, internally we have created our own incremental strategy to allow custom merge actions. We have an extra property under meta called custom merge actions which allows sql.
The merge is pretty much just a cut down version of get_incremental_merge_sql |
|
Did you create your own merge macro to use it? |
|
@sd-db the person in dbt Labs product that pushed back on this is no longer there. We should consider bringing this up again. |
|
yes, i created my own version of the existing merge pattern cutting down the existing options to keep it simple |
|
@benc-db great 😄 |
|
@mi-volodin I'd hold off on rebasing, as they might still tell us not to merge it. I'm just suggesting its worth it for us to bring up in our product meetings with them to see if their position has softened. Thanks however for your eagerness to improve the adapter for others :) |
|
@benc-db ah 🥲 sorry, I've read it wrongly. Then keeping it on ice until there will be some news. At least now there's a proof that it's not only needed, but actually achieved (in a potentially non-uniform way if implemented by others from scratch) with the ad-hoc strategy overloading. For me it looks like quite a good argument to proceed. |


Resolves #949
Description
This PR extends the capability of Merge strategy configuration and allow to completely override actions block with an explicit statement.
E.g. it allows to define multiple actions with multiple conditions and tune this part at developer's convenience.
Checklist
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section.