Skip to content

Feat(redshift): Add native Merge support for Redshift#3607

Merged
themisvaltinos merged 4 commits intomainfrom
themis/redshift_merge
Jan 10, 2025
Merged

Feat(redshift): Add native Merge support for Redshift#3607
themisvaltinos merged 4 commits intomainfrom
themis/redshift_merge

Conversation

@themisvaltinos
Copy link
Contributor

This update adds native MERGE support for Incremental by Unique Key models in Redshift (fixes: #3316)

This required adjustments to the generated statement to accommodate for the lack of alias support for the target table and limitations of the when matched clause Redshift MERGE Documentation

@erindru
Copy link
Collaborator

erindru commented Jan 9, 2025

This theoretically looks good but are you able to show the Redshift integration tests still passing by commenting out the branch filter for the cloud tests in .circleci/continue_config.yml? (and re-instating it prior to merge)

LogicalMergeMixin had less syntax limitations so I wonder if "upgrading" to Redshift's limited version of MERGE will break things

@themisvaltinos
Copy link
Contributor Author

This theoretically looks good but are you able to show the Redshift integration tests still passing by commenting out the branch filter for the cloud tests in .circleci/continue_config.yml? (and re-instating it prior to merge)

Thanks for this suggestion I tested it and it appears to be working as expected

LogicalMergeMixin had less syntax limitations so I wonder if "upgrading" to Redshift's limited version of MERGE will break things

I was wondering of this as well, but given that logical merge does not support when_matched or merge_filter I was thinking it should be fine in that aspect. But can you think of any other potential risks we should consider?

return expression

# Redshift does not support multiple "WHEN MATCHED" clauses.
if len(whens.expressions) != 2:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The !=2 check makes some assumptions about how this code is called.

Would counting the exp.When objects where matched=True and matched=False and erroring if there is more than 1 of each be more robust?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes good point also more future proof in case we change how we construct when_matched. Revised it to check that there are only one exp.When with matched=True and one with matched=False

@erindru
Copy link
Collaborator

erindru commented Jan 10, 2025

I tested it and it appears to be working as expected

Thanks, looks good!

But can you think of any other potential risks we should consider?

Every change carries risk :) but in this case, our tests pass so I think we are good. If this change breaks someones workflow, its an opportunity to improve our test suite

@themisvaltinos themisvaltinos merged commit 3487a0f into main Jan 10, 2025
3 checks passed
@themisvaltinos themisvaltinos deleted the themis/redshift_merge branch January 10, 2025 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants