GH-33973: [Python][Docs] Update documentation for Parquet filter keyword

[Fokko]

Rationale for this change

I wrote a converter from an arbitrary expression to DNF, but this was not needed after learning that it just accepts an expression now.

Closes #33973

[github-actions]

• Closes: [Python] Parquet filter keyword of read_table docstring is outdated #33973

[github-actions]

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

[westonpace]

I don't have a problem with this change but wonder if the root cause might be a little more fundamental. I wonder if ParquetDataset itself should be deprecated. The docs you probably want are pyarrow.dataset.dataset and pyarrow.dataset.Dataset (though filter would be provided by pyarrow.dataset.Dataset.to_table which unfortunately redirects its documentation to pyarrow.dataset.Scanner.from_dataset 😰 )

[Fokko]

@westonpace Thanks for the reply!

The docs you probably want are pyarrow.dataset.dataset and pyarrow.dataset.Dataset

I would agree with you, but I went for a lower API because I want to re-use the connection. This avoids another HEAD get request to S3. More background is provided here, and the dataset only accepts paths. If you think it is worthwhile to accept NativeFile there as well, let me know, and happy to raise a PR.

which unfortunately redirects its documentation to pyarrow.dataset.Scanner.from_dataset 😰 )

Do you want me to create a PR to copy those docs? Because of the redirect, the arguments are also not showing up in PyCharm. Let me know and I'll create a PR.

[westonpace]

Do you want me to create a PR to copy those docs? Because of the redirect, the arguments are also not showing up in PyCharm. Let me know and I'll create a PR.

Yes, I think that is a good idea but I might CC @jorisvandenbossche or @amol- to weigh in on whether they know some better way to avoid the duplication or have a preference here (I normally focus on the C++ end of things.)

[jorisvandenbossche]

@Fokko thanks for the catch! You are currently updating the docstring of ParquetDataset, but we should do the same update for read_table:

arrow/python/pyarrow/parquet/core.py

Line 2775 in 78a8da4

filters : List[Tuple] or List[List[Tuple]] or None (default)

(I think it's also read_table you are using in PyIceberg, and not ParquetDataset?)

Do you want me to create a PR to copy those docs? Because of the redirect, the arguments are also not showing up in PyCharm. Let me know and I'll create a PR.

Yes, I think that is a good idea but I might CC @jorisvandenbossche or @amol- to weigh in on whether they know some better way to avoid the duplication or have a preference here (I normally focus on the C++ end of things.)

Yeah, I think we should prefer some duplication if that gives better docstrings. I agree the indirection for the user right now isn't very user friendly.
We might be able to share some part of the docstring and inject that in multiple places to avoid duplicating the actual content, if that doesn't make things too complicated.

[Fokko]

@jorisvandenbossche Updating table one is a good suggestion indeed. Updated that one as well 👍🏻

(I think it's also read_table you are using in PyIceberg, and not ParquetDataset?)

Currently, we use the read_table indeed. I've also played around with the ParquetDataset, and it looked very similar. However, we don't need the lazy nature of the dataset, so directly loading a table makes more sense in our situation.

Yeah, I think we should prefer some duplication if that gives better docstrings. I agree the indirection for the user right now isn't very user friendly.
We might be able to share some part of the docstring and inject that in multiple places to avoid duplicating the actual content, if that doesn't make things too complicated.

Makes a lot of sense, I think sharing would be best. Let me create a separate PR for that

[Fokko]

@jorisvandenbossche first a bit of cleanup in #34034

[jorisvandenbossche]

One more thing about the docstring: for both read_table and ParquetDataset, the docstring also injects the content of _DNF_filter_doc (which holds the actual explanation of how the filter is expressed in this list of tuples form). We should probably update that to start with saying that the filter can either be a pyarrow Expression, or this DNF list of tuples form.

[Fokko]

@jorisvandenbossche Good call. I've updated the docstring and added an example.

[Fokko]

@jorisvandenbossche WDYT?

[jorisvandenbossche]

Thanks!

[ursabot]

Benchmark runs are scheduled for baseline = e63215c and contender = 306026d. 306026d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.58% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.16% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 306026d8 ec2-t3-xlarge-us-east-2
[Failed] 306026d8 test-mac-arm
[Finished] 306026d8 ursa-i9-9960x
[Finished] 306026d8 ursa-thinkcentre-m75q
[Finished] e63215ca ec2-t3-xlarge-us-east-2
[Failed] e63215ca test-mac-arm
[Finished] e63215ca ursa-i9-9960x
[Finished] e63215ca ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[ursabot]

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm