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

MERGE Strategy - Unique Key vs Merge Columns #25

Closed
arosychuk opened this issue Mar 22, 2022 · 4 comments
Closed

MERGE Strategy - Unique Key vs Merge Columns #25

arosychuk opened this issue Mar 22, 2022 · 4 comments
Labels
bug Something isn't working refactor Refactor a bit of code

Comments

@arosychuk
Copy link
Contributor

arosychuk commented Mar 22, 2022

The current merge strategy has a problem.

It uses a variable called merge_columns to build the join in the merge, which upon later inspection of other adapters, this is done with the unique_key, and the merge_columns is the columns to actually be updated (while the rest are not).

My naïve approach when I added this functionality is creating this issue, and it should be refactored, as a breaking change.

  1. Use the unique_key for the source and destination join
  2. Support the merge_columns to only merge the columns specified

In some cases, the unique_key could be an list instead of a string, so that would also need to be supported.

@arosychuk arosychuk added bug Something isn't working refactor Refactor a bit of code labels Mar 22, 2022
@arosychuk
Copy link
Contributor Author

@andyreagan - Any thoughts about how we can handle this change without causing too much chaos?

@andyreagan
Copy link
Contributor

It probably makes sense to align with other adapters here. Having control over which columns to merge seems nice.

I don't use this feature, and I don't think that this adapter has widespread usage right now. That said, generally, breaking changes need to go with a major version update.

Overall - having control of merge columns seems like a nice-to-have, so we don't need to do this right away (add it to a list of planned changes for v2.x). On the other hand, it's probably safe to just change it now. Hmmmm.

How long has the merge strategy even been implemented? Is this one that you just fixed in the most recent round? If you just added this, I think it's safe to change it.

@arosychuk
Copy link
Contributor Author

I agree, breaking changes are a major release, and would be a 2.0 following the semver rules.

Agreed, I don't think there is widespread use. Worst case, it would be a config keyword search+replace. If it was upgraded without the change, jobs would fail as the unique_key would be missing.

Yes, this was the merge I implemented, back in in November 2020. Not seeing any feedback, issues, or comments about it I suspect it's not used by many if at all.

@andyreagan
Copy link
Contributor

I went ahead and updated the adapter to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working refactor Refactor a bit of code
Projects
None yet
Development

No branches or pull requests

2 participants