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

issue #2 - catch case where identical rows are used in succession for the upsert #120

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

dgcaron
Copy link
Contributor

@dgcaron dgcaron commented Aug 8, 2023

This PR applies the proposed changes as described in #2 before the change the new test case yielded the following result:

+------------------------------------------------------------------------------------------+-------------------------------------------------------------------------------------------+
|                                           df1                                            |                                            df2                                            |
+------------------------------------------------------------------------------------------+-------------------------------------------------------------------------------------------+
| Row(pkey=1, attr='A', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) | Row(pkey=1, attr='A', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) |
| Row(pkey=1, attr='A', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) | Row(pkey=2, attr='B', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) |
| Row(pkey=2, attr='B', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) | Row(pkey=4, attr='D', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) |
| Row(pkey=4, attr='D', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) |                                            None                                           |
+------------------------------------------------------------------------------------------+-------------------------------------------------------------------------------------------+

the update is equal to the existing record 1 so no action should be taken but a new record is created instead without ending the previous record with id . with the change applied the record is untouched

+------------------------------------------------------------------------------------------+-------------------------------------------------------------------------------------------+
|                                           df1                                            |                                            df2                                            |
+------------------------------------------------------------------------------------------+-------------------------------------------------------------------------------------------+
| Row(pkey=1, attr='A', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) | Row(pkey=1, attr='A', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) |
| Row(pkey=1, attr='A', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) | Row(pkey=2, attr='B', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) |
| Row(pkey=2, attr='B', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) | Row(pkey=4, attr='D', cur=True, effective_date=datetime.date(2019, 1, 1), end_date=None) ||
+------------------------------------------------------------------------------------------+-------------------------------------------------------------------------------------------+

@dgcaron
Copy link
Contributor Author

dgcaron commented Aug 8, 2023

This change only accounts for the duplicate rows, there is another issue with this statement but that requires a more elaborate change and i wanted to split the issues in two seperate PR's

@MrPowers
Copy link
Owner

@dgcaron - thanks for this contribution.

@danielbeach - can you take a look and let us know if this change looks alright to you?

Copy link
Owner

@MrPowers MrPowers left a comment

Choose a reason for hiding this comment

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

LGTM

@MrPowers MrPowers merged commit 5a93f4e into MrPowers:main Sep 27, 2023
@MrPowers
Copy link
Owner

there is another issue with this statement but that requires a more elaborate change and i wanted to split the issues in two seperate PR's

@dgcaron - can you please make another PR with the other fixes? I promise we'll get it reviewed and merged faster.

@dgcaron
Copy link
Contributor Author

dgcaron commented Sep 27, 2023

The changes proposed are explained #121.. fixing it properly requires a breaking change in the current call so you might want to give your thoughts before I start writing code :)

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.

4 participants