feat: df.join lsuffix and rsuffix support #1857
Conversation
product-auto-label
bot
added
size: m
Genesis929
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
product-auto-label
bot
added
size: l
Genesis929
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
Genesis929
added
the
kokoro:force-run
bigframes-bot
removed
the
kokoro:force-run
tests/system/small/test_dataframe.py
Outdated
| ["string_col", "int64_col", "int64_too"] | ||
| ].rename(columns={"int64_too": "int64_col"}) | ||
| pd_result = pd_df_a.join(pd_df_b, how=how, lsuffix="_l", rsuffix="_r") | ||
| print(pd_result) |
There was a problem hiding this comment.
Remove leftover print() statements.
PS. Adding --pdb to your pytest command line arguments makes dropping into a debugger to inspect variables really easy. https://docs.pytest.org/en/stable/how-to/failures.html#dropping-to-pdb-on-failures
| if how == "cross": | ||
| return |
There was a problem hiding this comment.
I think it'd be worth added a test that ValueError is not raise for this condition with a cross join.
There was a problem hiding this comment.
Cross join actually raise another error, match added.
| f"bigframes_left_col_name_{i}" if col_name != on else on_col_name | ||
| for i, col_name in enumerate(left_col_original_names) | ||
| ] | ||
| left.columns = pandas.Index(left_col_temp_names) |
There was a problem hiding this comment.
This seems dangerous. We haven't made a copy of self, so I'm uncomfortable with mutating it. If we must do this, then please either:
- make a copy of self first
- or put a
finallyblock that resets the names back to the original in case anything when wrong.
I prefer (1) since it's less likely to have problems in we're in a multi-threaded environment.
There was a problem hiding this comment.
Updated left = self.copy()
| f"bigframes_left_idx_name_{i}" for i in range(len(left_idx_original_names)) | ||
| ] | ||
| if left._has_index: | ||
| left.index.names = left_idx_names_in_cols |
There was a problem hiding this comment.
Same here. Mutating the index is dangerous. Can we avoid this?
There was a problem hiding this comment.
We need to avoid duplicates in names when join, or the reordering columns won't work, so for current join logic, we can't avoid this.
| f"bigframes_right_col_name_{i}" | ||
| for i in range(len(right_col_original_names)) | ||
| ] | ||
| right.columns = pandas.Index(right_col_temp_names) |
There was a problem hiding this comment.
We need to avoid duplicates in names when join, or the reordering columns won't work, so for current join logic, we can't avoid this.
| right_columns, | ||
| lsuffix: str = "", | ||
| rsuffix: str = "", | ||
| extra_col: typing.Optional[str] = None, |
There was a problem hiding this comment.
Please add a docstring explaining this extra_col parameter and when it is intended to be used.
bigframes/dataframe.py
Outdated
| final_col_names.append(f"{col_name}{rsuffix}") | ||
| else: | ||
| final_col_names.append(col_name) | ||
| self.columns = pandas.Index(final_col_names) |
There was a problem hiding this comment.
I think we should only be modifying self if we're doing an inplace operation, right? Why is self getting changed? Can we avoid this?
There was a problem hiding this comment.
In this function the self is actually combined_df, so it should be safe. Changed to self.copy for additional safety.
| bf_df_b = scalars_df_index.dropna()[ | ||
| ["string_col", "int64_col", "int64_too"] | ||
| ].rename(columns={"int64_too": "int64_col"}) | ||
| bf_result = bf_df_a.join(bf_df_b, how=how, lsuffix="_l", rsuffix="_r").to_pandas() |
There was a problem hiding this comment.
Can we add some checks that bf_df_a's column names and index names didn't get modified?
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕