Fix uninitialized reads in parquet chunked reader

[pmattione-nvidia]

This PR fixes a couple of uninitialized reads in parquet chunked reader.

We are trying to go from an array of size N to a list of offsets to the data referred to by the array of length N+1, where the last offset is the total offset to the end. The exclusive_scan() calls compute the offsets, but do an uninitialized read of array[N]. This introduces thrust counting_transform_iterator's to perform an indirection so that we don't read array[N]. Note that exclusive_scan() computes a prefix sum, so the uninitialized data in array[N] shouldn't have been used to compute anything anyway.

Checklist

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

[davidwendt]

I would be concerned about any performance impact this my incur.
Overall, I don't think this extra code is necessary since the read does not actual contribute to the result.
We have many places in libcudf that work this way with exclusive-scan.
See the discussion here #12667 (comment) and the proposed resolution to be fixed in CCCL here: NVIDIA/cccl#876

[pmattione-nvidia]

That CCCL resolution has been open for 3.5 years now; I doubt it is getting fixed. And UB is bad. And any overhead this adds is negligible compared to everything else needed for parquet reads (decompression, reading global device memory, etc.)

[davidwendt]

That CCCL resolution has been open for 3.5 years now; I doubt it is getting fixed. And UB is bad.

You are welcome to note your comments in the CCCL issue.
But I don't think we should try to work around this in general.

[pmattione-nvidia]

Also, this overhead isn't being applied to processing of the data itself, it's just overhead on calculating a few page offsets. The perf cost of this is negligible, UB should be avoided, and CCCL is not going to fix this (it may even require a branch in exclusive_scan(), so they may not even want to change anything there).

[nvdbaranec]

I would be concerned about any performance impact this my incur. Overall, I don't think this extra code is necessary since the read does not actual contribute to the result. We have many places in libcudf that work this way with exclusive-scan. See the discussion here #12667 (comment) and the proposed resolution to be fixed in CCCL here: NVIDIA/cccl#876

The counterargument here is that this isn't being applied to bulk data. It's being applied to metadata - dozens/hundreds of things (page structs) during the setup phase.

[davidwendt]

Also, this overhead isn't being applied to processing of the data itself, it's just overhead on calculating a few page offsets. The perf cost of this is negligible, UB should be avoided, and CCCL is not going to fix this (it may even require a branch in exclusive_scan(), so they may not even want to change anything there).

I don't want to workaround issues like this but prefer CCCL fix (or not) the issue in their library.
I think the risk here is not worth this change.
Although the read is UB, the value is discarded by CCCL and there is no risk of data corruption.
Perhaps this could even be a feature request for compute-sanitizer to detect the read value is not used.
Regardless, this has been covered in great detail in #12667 and you are welcome to add more comments to that closed issue.

[davidwendt]

Recommend closing this PR.