ARROW-7547: [C++][Dataset][Python] Add ParquetFileFormat options

[bkietz]

Add parquet reader and arrow reader options to file format

[github-actions]

https://issues.apache.org/jira/browse/ARROW-7547

[fsaintjacques]

I would have expected this to simply receive a parquet::ArrowReaderProperties and parquet::ReaderProperties is there a reason this is not the case and members are copied in a new struct which will diverge over time.

Also remove the #pragma once from file you didn't modify the content, maybe a separate patch.

[bkietz]

@fsaintjacques I implemented that (taking /.*Properties/ explicitly) initially, but: replicating those fields seemed more robust than documenting what fields would be ignored. For example: ReaderProperties::memory pool (which will be ignored in favor of ScanContext::pool) and ArrowReaderProperties::use threads (which is redundant since the scanner will already parallelize across threads.

Furthermore as noted above: it seems to me that ParquetFormat should expose a set of field names (rather than indices) to be read as dictionaries which conflicts directly with ArrowReaderProperties::read dictionary indices

[jorisvandenbossche]

I tried this out a bit, working nicely! Two main remarks:

The options you now added to ParquetFileFormat are reader options. But if we are going to start adding writing functionality as well, that might get into conflict with writer options ? Should they then still be part of the Format object?

You mentioned in an inline comment that we should rather use field names instead of indices. Fully agreed with that (in the cython parquet code, inside the reader class the names are converted into indices, so the user can pass names). Can this be done on the C++ side, or is there something blocking this?

[bkietz]

@jorisvandenbossche I have namespaced reader specific options within ParquetFileFormat so they won't conflict with writer options. I've also switched to using names to designate dictionary columns. batch_size is a general consideration of scans, so I've promoted to a scan option (@fsaintjacques testing that necessitated resolving https://issues.apache.org/jira/browse/ARROW-7338 )

PTAL

[nealrichardson]

Some notes; LMK if you want backup on the R stuff.

[jorisvandenbossche]

I have namespaced reader specific options

+1

[bkietz]

@fsaintjacques Since use_buffered_stream/buffer_size were introduced to resolve https://issues.apache.org/jira/browse/ARROW-6180 (multiple threads accessing a single RandomAccessFile), maybe this shouldn't be an option and instead should be configured automatically during a multithreaded scan?

[jorisvandenbossche]

I am not really familiar with those options. So if your comment about them is accurate, it indeed doesn't sound useful to expose them here.

[bkietz]

@fsaintjacques @jorisvandenbossche I was incorrect: these options are not related to threading. They are provided to reduce memory overhead in the case where large row groups would otherwise be mapped whole into memory. I'll reintroduce the options and update the doccomments

[nealrichardson]

ICYMI there's a bug in the Rcpp you added: https://github.com/apache/arrow/pull/6235/checks?check_run_id=458210010#step:5:1020

[nealrichardson]

One final question but otherwise I approve. I have some ideas for making the open_dataset interface nicer with these options, but I'll follow up with that separately.

[nealrichardson]

Why exactly is this method added in this PR? Doesn't seem related to ParquetFileFormat options. Also, it's not tested.

[bkietz]

This is a property on the parquet reader so initially I had in alongside the other options in this PR. However it's not a parquet specific concern and it's one that might need tweaking on a scan-by-scan basis so I extracted it to ScanOptions.

It's tested in C++, but I can certainly add an equivalent one in R