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.

[Mottl]

@Mottl I think this PR would be nice to have. Do you think you would have time to address the comment from @rok? Checking the strings also has it's shortcomings so I am interested in what do you think?

Sure! Sorry for being unresponsive. My reply

[rok]

Thanks for the update @Mottl ! Could you also rebase or at least resolve the merge conflict in python/pyarrow/parquet/core.py?

[rok]

It appears we had merged a PR that covered this issue in part.

arrow/python/pyarrow/parquet/core.py

Lines 1940 to 1947 in 63813b6

	filesystem, path = _resolve_filesystem_and_path(source, filesystem)
	if filesystem is not None:
	if not filesystem.get_file_info(path).is_file:
	raise ValueError(
	"the 'source' argument should be "
	"an existing parquet file and not a directory "
	"when the pyarrow.dataset module is not available"
	)

Can you see if your check can be consolidated with the existing one?