Skip to content

fix: forward write_options when write_parquet receives ParquetWriterOptions - #1761

Open
Rodrigo-Palma wants to merge 1 commit into
apache:mainfrom
Rodrigo-Palma:fix/write-parquet-forward-write-options
Open

Rodrigo-Palma wants to merge 1 commit into
apache:mainfrom
Rodrigo-Palma:fix/write-parquet-forward-write-options

Conversation

@Rodrigo-Palma

Copy link
Copy Markdown

Which issue does this PR close?

Closes #1760.

Rationale for this change

DataFrame.write_parquet accepts write_options, documents it and declares it in the @overload for the ParquetWriterOptions form, but that branch delegates without forwarding it:

if isinstance(compression, ParquetWriterOptions):
    if compression_level is not None:
        msg = "compression_level should be None when using ParquetWriterOptions"
        raise ValueError(msg)
    self.write_parquet_with_options(path, compression)   # write_options dropped
    return

write_parquet_with_options(path, options, write_options=None) takes the parameter, so everything in DataFrameWriteOptions (partition_by, single_file_output, insert_operation, sort_by) was silently ignored for that one spelling. Measured with the same DataFrameWriteOptions(partition_by="part") in all three calls:

write_parquet(path, ParquetWriterOptions(), write_options=wo)        ['IDuOjvMa3pdEDotb_0.parquet']
write_parquet_with_options(path, ParquetWriterOptions(), ...=wo)     ['part=a', 'part=b']
write_parquet(path, "zstd", write_options=wo)                        ['part=a', 'part=b']

Only the ParquetWriterOptions branch loses the Hive partitioning, with no error and no warning, so the files just land in the wrong layout.

The branch came in ef62fa8 (#1169) while write_options was added earlier in #857, so the new delegation path was written without carrying the existing parameter over. Note that the same if refuses compression_level with an explicit ValueError: arguments genuinely incompatible with this branch get rejected on purpose, and write_options was not rejected, only forgotten.

What changes are included in this PR?

One line: write_options is passed through to write_parquet_with_options. Plus a regression test, test_write_parquet_writer_options_keeps_write_options, next to test_write_parquet.

  • Without the change: AssertionError: assert ['9UNQ0jX8PC8GFZxz_0.parquet'] == ['part=a', 'part=b']
  • With it: 1 passed

python/tests/test_dataframe.py: 327 passed with the change against 326 on a clean tree, with the same 4 pre-existing errors in both runs (test_logical_plan, test_optimized_logical_plan, test_execution_plan, test_async_iteration_of_df), so the delta is exactly the new test. ruff 0.15.1 check and ruff format --check are clean on both files.

Measurement caveat: I ran the Python layer of this branch against the published datafusion 54.0.0 wheel rather than a locally built _internal, so the Rust side is the released one. The block being changed is byte-identical between that wheel and main, which I verified by diffing it, so the behaviour shown above is the behaviour on main.

Are there any user-facing changes?

Yes, in the sense that write_options now takes effect for this call shape, which is what the signature, the docstring and the overload already promise. No API change: no signature, name or default is touched.

…ptions

`write_parquet` takes `write_options`, documents it and declares it in
the `@overload` for the `ParquetWriterOptions` form, but that branch
calls `write_parquet_with_options(path, compression)` and drops it. The
destination accepts the parameter, so everything in
`DataFrameWriteOptions` (`partition_by`, `single_file_output`,
`insert_operation`, `sort_by`) was silently ignored for that one
spelling.

Measured with the same `DataFrameWriteOptions(partition_by="part")`:

    write_parquet(path, ParquetWriterOptions(), write_options=wo)
        -> ['IDuOjvMa3pdEDotb_0.parquet']     not partitioned
    write_parquet_with_options(path, ParquetWriterOptions(), write_options=wo)
        -> ['part=a', 'part=b']
    write_parquet(path, "zstd", write_options=wo)
        -> ['part=a', 'part=b']

No error and no warning: the files just land in the wrong layout.

The branch arrived in ef62fa8 (apache#1169) while `write_options` came
earlier in apache#857, so the new delegation path was written without carrying
the existing parameter over. The same `if` refuses `compression_level`
with an explicit `ValueError`, which shows that arguments incompatible
with this branch get rejected on purpose; `write_options` was not
rejected, only forgotten.
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.

write_parquet ignores write_options when compression is a ParquetWriterOptions

1 participant