GH-37470: [Python][Parquet] Add missing arguments to ParquetFileWriteOptions

[judahrand]

Rationale for this change

I think this may have been missed when this feature was added.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

• Closes: [Python][Parquet] write_page_index to be set for Datasets #37470

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - How to contribute patches

[mapleFU]

Would you mind create a Github issue, and rename the title to:

GH-{issue-id}: [Python][Parquet] ...

[github-actions]

⚠️ GitHub issue #37470 has been automatically assigned in GitHub to PR creator.

[mapleFU]

Would you mind add a test for write_page_index? Since we need to ensure page index would be written.

Besides, disable statistics for column can force only write offset-index, maybe we can just test them

[judahrand]

Would you mind add a test for write_page_index? Since we need to ensure page index would be written.

I've added a test for this.

[mapleFU]

Great! I'll take a careful round later. By the way we can disable statistics to just disable Column Index but write offset index. Maybe this could be add to doc later.

[judahrand]

Great! I'll take a careful round later. By the way we can disable statistics to just disable Column Index but write offset index. Maybe this could be add to doc later.

Are you saying write_statistics=False, wrtie_page_index=True will write the OffsetIndex but not the ColumnIndex? If that's the case then I'd absolutely agree the docs should be updated as currently they suggest that write_statistics is ignored if write_page_index=True! But this should probably be a separate PR as the docs will need updating in pyarrow.parquet too?

[judahrand]

Great! I'll take a careful round later. By the way we can disable statistics to just disable Column Index but write offset index. Maybe this could be add to doc later.

Are you saying write_statistics=False, wrtie_page_index=True will write the OffsetIndex but not the ColumnIndex? If that's the case then I'd absolutely agree the docs should be updated as currently they suggest that write_statistics is ignored if write_page_index=True! But this should probably be a separate PR as the docs will need updating in pyarrow.parquet too?

That is indeed what you were saying as the test shows! That behaviour strikes me as quite unintuitive!

[mapleFU]

The logic is ok here, however, I think we should export the flag to parquet/core.py, and from that to _dataset_parquet.pyx. You can take a look at your another patch ( #37665 ). Since just a _dataset_parquet config is hard to use for users.

[mapleFU]

That is indeed what you were saying as the test shows! That behaviour strikes me as quite unintuitive!

Cause I'm the author of that behavior lol ( #35958 )

When disable collect statistics, currently it would be hard to collect the ColumnIndex, because ColumnIndex relies on Statistics.

Also, by the way, by default, if column index is enabled, the page header statistics will not be written. (Since spec says if column index exists, page header is not tent to be written)

[judahrand]

When disable collect statistics, currently it would be hard to collect the ColumnIndex, because ColumnIndex relies on Statistics.

Yeah, that makes sense! Would be good to clarify in the docs - will see if I get to it in another PR.

Also, by the way, by default, if column index is enabled, the page header statistics will not be written. (Since spec says if column index exists, page header is not tent to be written)

Yeah, what gets written and what doesn't is quite confusing. In fact the spec doesn't techincally say that the page-level statistics when writing the ColumnIndex but that it isn't recommended (one might want both in order to support old readers).
https://github.com/apache/parquet-format/blob/master/PageIndex.md#technical-approach

It's probably sensible default behaviour but it'd be nice to force being able to write both.

[judahrand]

The logic is ok here, however, I think we should export the flag to parquet/core.py, and from that to _dataset_parquet.pyx. You can take a look at your another patch ( #37665 ). Since just a _dataset_parquet config is hard to use for users.

The flags are already there. They were just never added to the ParquetFileWriteOptions:

arrow/python/pyarrow/parquet/core.py

Line 976 in e56726c

write_page_index=False,

[mapleFU]

Sorry for late reply. Finally I got this. User can write the data with these options in ParquetWriter python sdk, but cannot use them with ParquetFileWriteOptions.

This is LGTM.

@jorisvandenbossche @AlenkaF Would you mind take a look

[AlenkaF]

LGTM 👍

[AlenkaF]

One thought: would it be worth adding docstrings for ParquetFileWriteOptions and adding examples of use to the Python user guide? As a separate issue?

[mapleFU]

Any updates? Should we rerun continuous-integration/appveyor/pr and checkin if it's a unrelated flacky test?

[AlenkaF]

Any updates? Should we rerun continuous-integration/appveyor/pr and checkin if it's a unrelated flacky test?

The failing AppVeyor test would be fixed with rebasing the branch (#37555).

It would be good to see if something can be done regarding the documentation (see) before merging.

[judahrand]

Any updates? Should we rerun continuous-integration/appveyor/pr and checkin if it's a unrelated flacky test?

The failing AppVeyor test would be fixed with rebasing the branch (#37555).

Thanks, I've done this now.

It would be good to see if something can be done regarding the documentation (see) before merging.

I can try to make time to get to this but it feels like a pretty independent issue - especially as the WriteOptions classes don't even appear in the Python docs: https://arrow.apache.org/docs/python/api/dataset.html#classes. It feels like it would make more sense to add documentation for IPC, CSV, ORC, and Parquet FileWriteOptions all in one PR?

[AlenkaF]

I can try to make time to get to this but it feels like a pretty independent issue - especially as the WriteOptions classes don't even appear in the Python docs: https://arrow.apache.org/docs/python/api/dataset.html#classes. It feels like it would make more sense to add documentation for IPC, CSV, ORC, and Parquet FileWriteOptions all in one PR?

Agree totally, it deserves a special issue for sure! What you propose sounds great, thank you!
It would be enough for now just to open a separate issue and link it from here, so we do not forget.

[judahrand]

Agree totally, it deserves a special issue for sure! What you propose sounds great, thank you! It would be enough for now just to open a separate issue and link it from here, so we do not forget.

#37800

[mapleFU]

All test passed now, can we try to checkin this patch? :-)

[AlenkaF]

Great, will merge 👍

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit f52ebbb.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.