Skip to content

Fix loading parquet files with empty row groups - #8742

Open
Sriram-PR wants to merge 2 commits into
huggingface:mainfrom
Sriram-PR:fix-parquet-empty-row-group
Open

Sriram-PR wants to merge 2 commits into
huggingface:mainfrom
Sriram-PR:fix-parquet-empty-row-group

Conversation

@Sriram-PR

Copy link
Copy Markdown

Fixes #8740.

The parquet builder sized its batches on the first row group, parquet_fragment.row_groups[0].num_rows. When that row group is empty the batch size is 0 and pyarrow raises ArrowInvalid: BatchSize must be greater than 0. pandas writes an empty DataFrame as one row group with 0 rows, so one empty shard was enough to break load_dataset for the whole dataset, in both regular and streaming mode. A file whose first row group is empty but has data after it couldn't be loaded either.

Changes:

  • Parquet._generate_tables takes the batch size from the first non-empty row group and skips a fragment when all its row groups are empty. Files that loaded before get the same batch size as before, and batch_size from the config still takes precedence.
  • FolderBasedBuilder had the same line for metadata.parquet. It now uses the first non-empty row group too, with the existing DEFAULT_MAX_BATCH_SIZE fallback.

Tests:

  • test_parquet_empty_row_groups (regular and streaming) loads an empty file, a normal file, and a file with an empty first row group followed by data, and checks the rows.
  • test_data_files_with_empty_row_group_in_metadata_parquet covers the metadata.parquet case.

All 3 fail on main and pass with this change. I also checked by hand that reshard() on a file with an empty first row group gives the right rows. tests/packaged_modules/ has the same failures as on main (21, all torch and other optional deps I don't have installed). make quality passes.

@chrikrah chrikrah left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Sriram-PR approving at b663084: the three new tests fail with parquet.py and folder_based_builder.py reverted to the merge base and pass with the change, and nothing else in either test module moves.

$ python -m pytest tests/packaged_modules/test_parquet.py tests/packaged_modules/test_folder_based_builder.py -q   # tree at b663084, pyarrow 25.0.1
67 passed in 2.34s
$ # same command, both source files reverted to 9e7496a (merge base), tests kept
FAILED tests/packaged_modules/test_parquet.py::test_parquet_empty_row_groups[False]
FAILED tests/packaged_modules/test_parquet.py::test_parquet_empty_row_groups[True]
FAILED tests/packaged_modules/test_folder_based_builder.py::test_data_files_with_empty_row_group_in_metadata_parquet
3 failed, 64 passed in 2.39s
$ ruff check <4 files> && ruff format --check <4 files>   # ruff 0.16.10
All checks passed!
4 files already formatted

non-blocking: a dataset made only of empty Parquet files is new behaviour that no test pins. Non-streaming raises and streaming returns 0 rows with the schema, where 9e7496a crashed in both modes. Worth one case in test_parquet_empty_row_groups, so the difference between the two modes is pinned on purpose?

$ python p8742.py   # b663084, data_files=["empty.parquet"] only, both streaming values
only-empty streaming=False ValueError: Instruction "train" corresponds to no data!
only-empty streaming=True rows=0 features={'a': Value('int64'), 'b': Value('string')}

@lhoestq, could you take a look when you have a moment? It closes #8740 and touches only the two batch-size lines.

@Sriram-PR

Copy link
Copy Markdown
Author

@lhoestq gentle ping on this one when you have time. It fixes #8740 (parquet files with an empty row group, which pandas writes for an empty DataFrame). CI hasn't run yet since it needs workflow approval.

@Sriram-PR

Copy link
Copy Markdown
Author

@lhoestq another ping on this one, since it seems to have slipped past the last few merge rounds. It's a small fix for #8740 (loading a parquet file with an empty row group, e.g. one pandas writes for an empty DataFrame) and still merges cleanly with main. CI just needs workflow approval. I can merge main or rework it if you want it done differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parquet file with an empty row group fails to load: ArrowInvalid: BatchSize must be greater than 0

2 participants