GH-46374: [Python][Docs] Adds type checks for source in read_table

[Mottl]

Rationale for this change

pyarrow.parquet.read_table actually supports source parameter as a list of strings

Are these changes tested?

No, an issue to test those has been opened as this code path is currently not being tested. See:

• [Python] Exercise fallback case on tests for parquet.read_table in case dataset is not available #46373

Are there any user-facing changes?

no, only docstring

• GitHub Issue: [Python][Doc] Improve docs to specify that source argument on parquet.read_table can also be a list of strings #46374

[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}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[raulcd]

This is tricky because if the dataset module is enabled, yes we can use a list of strings but if we've built pyarrow without dataset a list of strings won't work because instead of using ParquetDataset->source we will be using ParquetFile->source.
I don't think we want to mislead users thinking a list of strings can be used when it might not be the case. cc @AlenkaF

[rok]

@raulcd I suppose we want to document this conditional behavior?

[raulcd]

I suppose we want to document this conditional behavior?

That makes sense, then something in the lines:
If the dataset module is enabled a list of strings can also be used which correspond to a list of files.
or something in the lines? @rok ?

[AlenkaF]

I agree that the conditional behavior is worth documenting.

If the dataset module is enabled a list of strings can also be used which correspond to a list of files.

This is great. I would just suggest changing "can also be used" to "can also be passed" to align more closely with the existing phrasing.

One more thing to note: this will require a test that would use the dataset mark. Because of that, the PR no longer fits into the MINOR category and will need a corresponding issue to track the change.

[rok]

Agreed with the proposed text (as amended by Alenka).

For the dataset marked test - might as well be a separate PR?

[Mottl]

Can we just concat_tables from multiple ParquetFiles in case ParquetDataset is unavailable? Reading many files at once is a really handy feature.

[AlenkaF]

I don't think we want to go in that direction—concatenation would introduce complexity that's already handled in the Dataset C++ layer (schema mismatches is a good example). Instead, we could consider adding a warning and suggest the use of the dataset module when passing files as a list (or a dict) here:

arrow/python/pyarrow/parquet/core.py

Line 1828 in a2e6136

# TODO test that source is not a directory or a list

[Mottl]

Okay, close this PR as soon as you finish with the discussion. Thanks all!

[raulcd]

Hi @Mottl thanks for your PR and contribution, I think the discussion is closed. Would you like to update this PR with docstring clarification based on the feedback and add a test as @AlenkaF commented? If you can't or don't have time we can add an issue to the issue tracker

[Mottl]

Have a look please

[Mottl]

@raulcd

[github-actions]

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

[github-actions]

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

[raulcd]

@AlenkaF @rok I've opened an issue to test this code path as is not tested currently, let me know if that makes sense to you.
The current checks and docstring look good to me.

[rok]

What if source is a folder in an S3 bucket? os.path.isdir
will return True for existing folder. Will it check through the s3 fs? That would add a network call. Can we just check if the string is a valid folder path and not if it also exists?