Fix IterableDataset.remove_columns when a single column name is passed - #8384
Open
LeSingh1 wants to merge 1 commit into
Open
Fix IterableDataset.remove_columns when a single column name is passed#8384LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
…atures
remove_columns accepts either a single column name or a list, but it only ever
tested `col in column_names`. With a string argument that is a substring test,
so every existing column whose name is contained in the one being removed was
deleted from info.features while its values were still yielded:
ds = Dataset.from_dict({"label": [0, 1], "label_text": ["a", "b"]})
it = ds.to_iterable_dataset().remove_columns("label_text")
it.column_names # [] instead of ["label"]
next(iter(it)) # {"label": 0}
features and column_names then disagree with the actual examples, which breaks
anything reading the schema afterwards (select_columns, cast, writing).
Normalize a string argument to a list first, like select_columns already does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IterableDataset.remove_columnsaccepts either a single column name or a list, but it only ever testscol in column_names. When a string is passed, that becomes a substring test, so any existing column whose name is contained in the one being removed is dropped frominfo.features:featuresandcolumn_namesthen disagree with the actual examples, which breaks anything that reads the schema afterwards —select_columns,cast, or writing to parquet. The map-styleDataset.remove_columns("label_text")correctly returns['label'], so the two paths disagree.The existing
test_iterable_dataset_remove_columnspasses only incidentally: it removes"id"and no sibling column name contains"id".This normalizes a string argument to a list first, which is what
select_columnsin the same class already does.Added coverage for the single-string case with a sibling whose name is a substring; it fails on
mainwithassert [] == ['label']and passes with the change.tests/test_iterable_dataset.pyandtests/test_dataset_dict.pypass (531 passed, 30 skipped; the 2 remaining failures are pre-existing network-dependent tests unrelated to this change).