Skip to content
This repository was archived by the owner on Apr 1, 2026. It is now read-only.

feat: support inplace=True in rename and rename_axis#1744

Merged
tswast merged 6 commits intomainfrom
b329460931-rename-inplace
May 30, 2025
Merged

feat: support inplace=True in rename and rename_axis#1744
tswast merged 6 commits intomainfrom
b329460931-rename-inplace

Conversation

@tswast
Copy link
Copy Markdown
Contributor

@tswast tswast commented May 16, 2025

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes internal issue b/329460931 🦕

@product-auto-label product-auto-label bot added size: l Pull request size is large. api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. labels May 16, 2025
@tswast
Copy link
Copy Markdown
Contributor Author

tswast commented May 16, 2025 •
edited
Loading

  • Missed Index.rename. Will mark as ready once that's up.

Edit. Done!

@tswast tswast marked this pull request as ready for review May 16, 2025 22:16
@tswast tswast requested review from a team and TrevorBergeron May 16, 2025 22:16
@tswast
Copy link
Copy Markdown
Contributor Author

tswast commented May 19, 2025 •
edited
Loading

Re: failures

____________________________ test_loc_list_nameless ____________________________
[gw2] linux -- Python 3.9.20 /tmpfs/src/github/python-bigquery-dataframes/.nox/system-3-9/bin/python

scalars_df_index =           bool_col                                          bytes_col  \
rowindex                                     ....999999+00:00  
8                         T             <NA>                              <NA>  

[9 rows x 13 columns]
scalars_pandas_df_index =           bool_col  ...                     timestamp_col
rowindex            ...                                  
0 ... ...  2038-01-19 03:14:17.999999+00:00
8            False  ...                              <NA>

[9 rows x 13 columns]

    def test_loc_list_nameless(scalars_df_index, scalars_pandas_df_index):
        index_list = [0, 0, 0, 5, 4, 7]
    
        bf_series = scalars_df_index.string_col.rename(None)
        bf_result = bf_series.loc[index_list]
    
        pd_series = scalars_pandas_df_index.string_col.rename(None)
        pd_result = pd_series.loc[index_list]
    
>       pd.testing.assert_series_equal(
            bf_result.to_pandas(),
            pd_result,
        )
E       AssertionError: Series.index are different
E       
E       Attribute "names" are different
E       [left]:  [None]
E       [right]: ['rowindex']

I think I need to call .copy() before I do anything inplace.

Edit: this hypothesis seems to be incorrect. I don't add any system tests for inplace, only unit tests. Instead, it was the problem identified with tuples.

tswast
tswast commented May 19, 2025
View reviewed changes
bigframes/core/indexes/base.py
)

def rename(self, name: Union[str, Sequence[str]]) -> Index:
names = [name] if isinstance(name, str) else list(name)
Copy link
Copy Markdown
Contributor Author

@tswast tswast May 19, 2025

Choose a reason for hiding this comment

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

I think this change to Label (Hashable) is what's breaking some MultiIndex tests. Need a better way to check if something is already a sequence / iterable and not a string.

Copy link
Copy Markdown
Contributor Author

@tswast tswast May 19, 2025

Choose a reason for hiding this comment

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

Fixed in 45f9232

I think we want to support integer names, too, so instead I specifically exclude tuple from the check for hashable objects.

TrevorBergeron
TrevorBergeron previously approved these changes May 19, 2025
View reviewed changes
@tswast
Copy link
Copy Markdown
Contributor Author

tswast commented May 19, 2025 •
edited
Loading

e2e failures:

FAILED tests/system/small/ml/test_linear_model.py::test_linear_reg_model_global_explain
FAILED tests/system/small/ml/test_preprocessing.py::test_label_encoder_series_default_params
FAILED tests/system/small/ml/test_preprocessing.py::test_one_hot_encoder_save_load
FAILED tests/system/small/ml/test_preprocessing.py::test_standard_scaler_normalizes
FAILED tests/system/small/ml/test_preprocessing.py::test_label_encoder_params
FAILED tests/system/small/ml/test_preprocessing.py::test_standard_scaler_normalizeds_fit_transform
FAILED tests/system/small/ml/test_llm.py::test_llm_gemini_pro_score_params[gemini-1.5-pro-002]

All of these seem to be timeout related with BQML integration. I don't think it should have been caused by my changes.

Presubmits are still running, but the 3.9 system tests passed.

@tswast tswast enabled auto-merge (squash) May 19, 2025 23:21
@tswast tswast changed the title feat: support inplace=True in rename and rename_axis feat: support inplace=True in rename and rename_axis May 19, 2025
@tswast tswast merged commit 734cc65 into main May 30, 2025
18 of 24 checks passed
@tswast tswast deleted the b329460931-rename-inplace branch May 30, 2025 21:16
@release-please release-please bot mentioned this pull request May 30, 2025
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@TrevorBergeron TrevorBergeron TrevorBergeron approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@liujiashang liujiashang

Labels

api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. size: l Pull request size is large.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tswast @TrevorBergeron @liujiashang