More granular column selection in ORC reader

[vuule]

Closes #8848

• Allows caller to specify nested column paths, so that the fields not listed in the columns parameter are excluded.
• The order of fields/columns in the output table is consistent with the order of paths/names in the columns parameter.
• Moved aggregate_orc_metadata implementation to a separate file (can be .cpp!)
• Add tests to cover different cases with a mix of nested and parent columns selection.
• changed a few fields from uint32_t to int32_t to avoid unsigned arithmetic.

[codecov]

Codecov Report

Merging #9496 (3c0d862) into branch-21.12 (ab4bfaa) will decrease coverage by 0.12%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-21.12    #9496      +/-   ##
================================================
- Coverage         10.79%   10.66%   -0.13%     
================================================
  Files               116      117       +1     
  Lines             18869    19725     +856     
================================================
+ Hits               2036     2104      +68     
- Misses            16833    17621     +788     

Impacted Files	Coverage Δ
python/dask_cudf/dask_cudf/sorting.py	92.90% <0.00%> (-1.21%)	⬇️
python/cudf/cudf/io/csv.py	0.00% <0.00%> (ø)
python/cudf/cudf/io/hdf.py	0.00% <0.00%> (ø)
python/cudf/cudf/io/orc.py	0.00% <0.00%> (ø)
python/cudf/cudf/__init__.py	0.00% <0.00%> (ø)
python/cudf/cudf/_version.py	0.00% <0.00%> (ø)
python/cudf/cudf/core/abc.py	0.00% <0.00%> (ø)
python/cudf/cudf/api/types.py	0.00% <0.00%> (ø)
python/cudf/cudf/io/dlpack.py	0.00% <0.00%> (ø)
python/cudf/cudf/core/frame.py	0.00% <0.00%> (ø)
... and 66 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0951ba...3c0d862. Read the comment docs.

[robertmaynard]

CMake changes 👍

[rgsl888prabhu]

First pass, looks pretty good, just had few questions.

[vyasr]

Good to see more code in cpp files, and I like the new feature (although I had a question about nesting depth that I've left inline). One overarching question: there's a lot of int32_t usage in this PR, is that because of the ORC specification or should the code be making use of cudf::size_type? Aside from that, I don't know this code too well so I left some questions in places that I didn't understand and comments on things that weren't really changes in this PR. Hopefully they're not too troublesome, feel free to ignore things that you need to.

[vuule]

@vyasr Thank you for the detailed review :)
Regarding

there's a lot of int32_t usage in this PR, is that because of the ORC specification or should the code be making use of cudf::size_type?

I did considered this option. My understanding is that size_type is used to represent the number of rows/row index. The uses here are all column indices. I wasn't sure is size_type applied there too, so I left as int32_t. ORC specs suggest uint32_t, which is more error-prone etc. Open for suggestions, maybe there should even be a new alias in use here.
Edit: switched to size_type :)

[vyasr]

@vyasr Thank you for the detailed review :) Regarding

there's a lot of int32_t usage in this PR, is that because of the ORC specification or should the code be making use of cudf::size_type?

I did considered this option. My understanding is that size_type is used to represent the number of rows/row index. The uses here are all column indices. I wasn't sure is size_type applied there too, so I left as int32_t. ORC specs suggest uint32_t, which is more error-prone etc. Open for suggestions, maybe there should even be a new alias in use here. Edit: switched to size_type :)

Just following up even though you did already make the change: I think size_type is also appropriate for column indices. My reasoning is that when we index into columns the expected input is a size_type (for example, via table_view::column), and size_type defines the bounds for the number of columns. I've also seen enough code switching from raw for loops to algorithms where the original for loops used size_type to make me think that was the expected use case for the type.

[vyasr]

Thanks for answering my questions! This PR LGTM, but would probably benefit from an additional CPP review from a cuIO expert.

[vuule]

Thanks for answering my questions! This PR LGTM, but would probably benefit from an additional CPP review from a cuIO expert.

@rgsl888prabhu is the expert on this code, so this should be covered :)

[rgsl888prabhu]

Code looks good, only have couple of queries on testing.

[vuule]

@gpucibot merge