-
Notifications
You must be signed in to change notification settings - Fork 6
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 extra check if both values are NA in bulk_upsert #53
Conversation
@@ -391,7 +391,10 @@ def bulk_upsert_chunk(self, df: pd.DataFrame) -> None: | |||
for col in self.model_class.updateable_columns: | |||
updated_col = col + self.merge_suffix | |||
if updated_col in df.columns: | |||
cond.append(df[col] != df[updated_col]) | |||
df[updated_col] = df[updated_col].astype(df[col].dtype) |
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.
Maybe add a line explaining why this is necessary? And a test (in a GitHub Action that explicitly installs pyarrow) to guard against regression?
Do we intend to release 0.7.1 immediately after this fix so that IAMconsortium/pyam#820 can move forward? |
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.
Looks good to me, provided the tests pass. But I'm not sure I like test names like "test / test (3.11, false) (pull_request) ". Doesn't seem as descriptive as it could be, but I don't have a tangible better solution of the top of my head.
Alright I added a test matrix entry for pyarrow and python 3.12. Also added some comments. |
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!
Fixes #50 's pyarrrow compatibility problem.