Use int64_t for the num_rows slot in parquet_reader_options

[wence-]

Description

Although reading a single table of more than size_type::max() rows is not possible, we may wish to specify options to read greater than size_type::max() rows from a large parquet file and require that the consumer of the options splits them up appropriately.

This is useful in chunked/streaming pipelines because we do not have to recreate the read_parquet options API and can instead get the caller to provide an options object that we pick apart.

Internally the reader implementation already uses int64 for num_rows, so this is really just a (breaking) interface change.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[bdice]

Do we need docs somewhere that explain the limitation (when reading, the row count must fit into size_type)? Maybe adding test for that error would be good. Otherwise LGTM.

[mhaseeb123]

We need to add the following check here to make sure we don't try to read more than 2B rows with read_parquet API:

cudf/cpp/src/io/parquet/reader_impl.cpp

Lines 877 to 884 in 7bbc981

	table_with_metadata reader_impl::read()
	{
	CUDF_EXPECTS(_output_chunk_read_limit == 0,
	"Reading the whole file must not have non-zero byte_limit.");
	prepare_data(read_mode::READ_ALL);
	return read_chunk_internal(read_mode::READ_ALL);
	}

CUDF_EXPECTS(std::cmp_less_equal(options.get_num_rows(), std::numeric_limits<cudf::size_type>::max(), "Reading the whole file at once must not read more rows than cudf's column size limits");

[mhaseeb123]

We need to add a check making sure we don't try to read more than 2B rows with the read_parquet() API.

Maybe adding test for that error would be good.

I think adding a test as well would also be a good idea.

[wence-]

We need to add a check making sure we don't try to read more than 2B rows with the read_parquet() API.

Maybe adding test for that error would be good.

I think adding a test as well would also be a good idea.

Done.

[wence-]

We need to add the following check here to make sure we don't try to read more than 2B rows with read_parquet API:

cudf/cpp/src/io/parquet/reader_impl.cpp

Lines 877 to 884 in 7bbc981

	table_with_metadata reader_impl::read()
	{
	CUDF_EXPECTS(_output_chunk_read_limit == 0,
	"Reading the whole file must not have non-zero byte_limit.");
	prepare_data(read_mode::READ_ALL);
	return read_chunk_internal(read_mode::READ_ALL);
	}

CUDF_EXPECTS(std::cmp_less_equal(options.get_num_rows(), std::numeric_limits<cudf::size_type>::max(), "Reading the whole file at once must not read more rows than cudf's column size limits");

Done, but handling the case that the num_rows is nullopt.

[mhaseeb123]

Done, but handling the case that the num_rows is nullopt.

Thanks. I don't really see the check in the reader_impl::read() currently. Perhaps a commit hasn't been pushed yet? I know that the read_parquet() does catch num_rows() being > 2B eventually while selecting row groups to read, but I would rather catch it early (right at the start of reader_impl::read()) and throw.

Also, in the test, can we read the same file (with same num_rows) using the chunked parquet reader (with finite and zero chunk and pass read limits) and make sure it does not throw.

Thanks for working on this again!

[wence-]

Done, but handling the case that the num_rows is nullopt.

Thanks. I don't really see the check in the reader_impl::read() currently. Perhaps a commit hasn't been pushed yet? I know that the read_parquet() will catch num_rows() being > 2B eventually while selecting row groups to read, but I would rather catch it early (right in reader_impl::read() and throw.

Ah sorry, too many in flight branches, done now.

[mhaseeb123]

Also, in the test, can we read the same file (with same num_rows) using the chunked parquet reader (with finite and zero chunk and pass read limits) and make sure it does not throw.

Thanks. This looks good to me, I can approve as soon as we add the chunked reader in the new test as well (maybe we can move this test to parquet_chunked_reader_test.cu if needed)

[mhaseeb123]

Minor stuff

[wence-]

Also, in the test, can we read the same file (with same num_rows) using the chunked parquet reader (with finite and zero chunk and pass read limits) and make sure it does not throw.

Thanks. This looks good to me, I can approve as soon as we add the chunked reader in the new test as well (maybe we can move this test to parquet_chunked_reader_test.cu if needed)

I think I did this.

[mhaseeb123]

Thank you for iterating on this. LGTM!

[wence-]

/merge