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

fix(rust, python): fix rename + projection pushdown #10624

Merged
merged 1 commit into from Aug 21, 2023
Merged

fix(rust, python): fix rename + projection pushdown #10624

merged 1 commit into from Aug 21, 2023

Conversation

ritchie46
Copy link
Member

fixes #10595

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Aug 20, 2023
@reswqa
Copy link
Collaborator

reswqa commented Aug 20, 2023

Nice fix!

BTW, I have a little concern about the case without optimization.

for (old, new) in existing.iter().zip(new.iter()) {
// the column might be removed due to projection pushdown
// so we only update if we can find it.
if let Some(dtype) = input_schema.get(old) {
if new_schema.with_column(new.clone(), dtype.clone()).is_none() {
new_schema.remove(old);
}
}
}

Looks like rename_schema may trigger incorrect order when swapping fields. 🤔

That's to say the following code panicked unexpected.

  def test_without_opt() -> None:
    lf = pl.LazyFrame(schema=["a", "b"])
    assert lf.select("a", "b").rename({"b": "a", "a": "b"}).select(
        "a"
    ).collect(projection_pushdown=False).schema == {"a": pl.Float32}

@ritchie46
Copy link
Member Author

Hmm... Is that panic dur to changes in this PR? Otherwise we have to fix that in a separate one.

Swapping renames bite me more often. :/

@reswqa
Copy link
Collaborator

reswqa commented Aug 21, 2023

Hmm... Is that panic dur to changes in this PR?

No, that panic is not introduced by this change. I am bringing it up here because the test case that can reproduce it is almost the same as in this PR, with the only difference being that optimization is turned off.

Otherwise we have to fix that in a separate one.

That's make sense! I will open a separate issue to cover it.

@ritchie46
Copy link
Member Author

That's make sense! I will open a separate issue to cover it.

Thanks!

@ritchie46 ritchie46 merged commit 15527ae into main Aug 21, 2023
25 checks passed
@ritchie46 ritchie46 deleted the rename branch August 21, 2023 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: ColumnNotFound error when renaming LazyFrame
2 participants