Parquet: Remove the row position since parquet row group has it natively

[flyrain]

After apache/parquet-java@c7bff51 shipped in parquet 1.12.3, the parquet row group provides the row index offset natively. We don't need to calculate it in Iceberg.

cc @chenjunjiedada @rdblue @wypoon @aokolnychyi

[flyrain]

Looks like the RowIndexOffset won't be set correct if we read from the middle of a parquet file. It will start from 0 no matter where we start to read.

[chenjunjiedada]

Looks like the RowIndexOffset won't be set correct if we read from the middle of a parquet file. It will start from 0 no matter where we start to read.

@flyrain Do you mean parquet doesn't give the right offset? I remember there is a fix(apache/parquet-java#978) for this, not sure whether this fix is already in 1.12.3.

[flyrain]

Yes, that's exactly why our test testReadRowNumbersWithSplits failed. apache/parquet-java#978 is not in 1.12.3(the latest parquet release). We may have to wait until it is released.

For example, if a file has two row groups with 10 rows each, and we attempt to only read the 2nd row group, we are going to produce row indexes 0, 1, 2, ..., 9 instead of expected 10, 11, ..., 19.

https://issues.apache.org/jira/browse/PARQUET-2161

[wypoon]

So this change needs to wait until there is a parquet-mr release with a fix for PARQUET-2161, right?
Just so I understand, for our usage, the bug manifests when Parquet.ReadBuilder#split is called (and thus when Parquet.ReadBuilder#build is called, the VectorizedParquetReader or ParquetReader is constructed with a ParquetReadOptions that has a ParquetMetadataConverter.RangeMetadataFilter), right? Why doesn't it affect the test I added in TestSparkReaderDeletes?

[flyrain]

So this change needs to wait until there is a parquet-mr release with a fix for [PARQUET-2161]

Yes.

Why doesn't it affect the test I added in TestSparkReaderDeletes?

I assume there is only one reader initialized in the test you added, which can set the right row offset since it reads from the beginning of the parquet file, instead of from the middle.

[wypoon]

Why doesn't it affect the test I added in TestSparkReaderDeletes?

I assume there is only one reader initialized in the test you added, which can set the right row offset since it reads from the beginning of the parquet file, instead of from the middle.

Ah, in a real-world situation, we could have a parquet file with multiple row groups and it is split among multiple scan tasks, and it has deletes to be applied, in which case the read will be incorrect then (since some split(s) will be reading the file from the middle).

[rdblue]

Do we trust this value from Parquet?

[flyrain]

Do we trust this value from Parquet?

The approach parquet used is similar to what @chenjunjiedada implemented in Iceberg repo. As long as it is reliable(no bug), I don't see a reason to not trust it. By using it, we don't have to calculate it again since parquet lib did it already, and it also makes the Iceberg code base a bit cleaner.

[flyrain]

We can reconsider this after this PR #7301 is merged.

[ricardopereira33]

Hi @flyrain @wypoon !

Are there any updates regarding this issue? We have a case when we write with Trino, and then we do data files compaction (or even just read a file), Spark can not read the file because the offsets are not within the range... example:

Code section: https://github.com/apache/parquet-mr/blob/0a066d8a5c71386e56dee7bd7a21170b27e4283a/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java#L1287

[Fokko]

@flyrain @chenjunjiedada do we want to revisit this since we're on Parquet 1.13.1?

[flyrain]

@Fokko, yes, we can revisit this with the new parquet release. It has the change Iceberg required. It should be safe and clean. Although, i'm not sure the issue mentioned by @ricardopereira33 is related. Hi @ricardopereira33, can you elaborate a bit?

[ricardopereira33]

Hi @flyrain ! I added all the details in this Slack thread. Can you see it?

[flyrain]

Thanks for sharing @ricardopereira33. Looks like the issue is due to a bug in Parquet 1.12.3, not directly related to this PR. It'd be nice to have this PR in though.

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

[github-actions]

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.