Parquet: Use native getRowIndexOffset support instead of calculating it #11520

wypoon · 2024-11-12T01:08:08Z

No description provided.

wypoon · 2024-11-12T02:42:14Z

@szehon-ho @flyrain can you please review?
cc @huaxingao

Fokko · 2024-11-12T11:58:52Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReader.java

@@ -28,5 +28,14 @@ public interface ParquetValueReader<T> {

  List<TripleIterator<?>> columns();

+  /**
+   * @deprecated since 1.6.0, will be removed in 1.7.0; use setPageSource(PageReadStore) instead.


Suggested change

* @deprecated since 1.6.0, will be removed in 1.7.0; use setPageSource(PageReadStore) instead.

* @deprecated since 1.8.0, will be removed in 1.9.0 or 2.0.0; use setPageSource(PageReadStore) instead.

In my change, I put down 1.9.0. If there is no 1.9.0, and the methods are removed in 2.0.0 instead, I don't think that would be a problem. On the other hand, we don't want to give the impression that the methods could still exist in a 1.9.0 and might be removed in 2.0.0 instead.

Fokko · 2024-11-12T11:59:10Z

parquet/src/main/java/org/apache/iceberg/parquet/VectorizedReader.java

@@ -43,10 +43,25 @@ public interface VectorizedReader<T> {
   * @param pages row group information for all the columns
   * @param metadata map of {@link ColumnPath} -&gt; {@link ColumnChunkMetaData} for the row group
   * @param rowPosition the row group's row offset in the parquet file
+   * @deprecated since 1.6.0, will be removed in 1.7.0; use setRowGroupInfo(PageReadStore,


Suggested change

* @deprecated since 1.6.0, will be removed in 1.7.0; use setRowGroupInfo(PageReadStore,

* @deprecated since 1.8.0, will be removed in 1.9.0 or 2.0.0; use setRowGroupInfo(PageReadStore,

wypoon · 2024-11-12T17:41:07Z

@Fokko thanks for reviewing!
I'd actually updated the deprecation comments locally but failed to commit the change before I pushed.

huaxingao · 2024-11-13T20:12:11Z

...k/v3.5/spark/src/main/java/org/apache/iceberg/spark/data/vectorized/ColumnarBatchReader.java

+  public void setRowGroupInfo(
+      PageReadStore pageStore, Map<ColumnPath, ColumnChunkMetaData> metaData) {
+    super.setRowGroupInfo(pageStore, metaData);
+    this.rowStartPosInBatch = pageStore.getRowIndexOffset().orElse(0L);


if pageStore.getRowIndexOffset() is empty, does it mean getRowIndexOffset() returns a negative value? Shall we throw Exception instead of default it to 0?

That is a good question.
As I understand it, the PageReadStore implementation (ColumnChunkPageReadStore) is normally constructed with the rowIndexOffset, but if the offset is not available then it is constructed with -1 for the rowIndexOffset. PageReadStore::getRowIndexOffset() will not return a negative value; it will return Optional.empty() in that case.
I suppose we can throw an IllegalArgumentException instead in such a situation, instead of setting rowStartPosInBatch to 0.
@flyrain do you have an opinion on this?
Is there someone who knows Parquet well who can confirm that in normal operation, PageReadStore::getRowIndexOffset() should not return Optional.empty()?

wypoon · 2024-11-14T21:31:30Z

@huaxingao @Fokko I have updated the PR; please review again.

huaxingao · 2024-11-15T19:22:03Z

arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorizedArrowReader.java

+              .getRowIndexOffset()
+              .orElseThrow(
+                  () ->
+                      new IllegalArgumentException(


nit: Is there a better Exception than IllegalArgumentException? Is IllegalStateException a bit better?

I can see why you might consider IllegalStateException. I do think IllegalArgumentException is appropriate, because the PageReadStore is an argument to the method being called, and the problem is with the PageReadStore. I think IllegalStateException is typically used to indicate an internal inconsistency in the module.
Consider if we use Guava's Preconditions to check a condition here. The condition would be that source.getRowIndexOffset().isPresent(). The checkArgument methods throw IllegalArgumentException and "Ensures the truth of an expression involving one or more parameters to the calling method." The checkState methods throw IllegalStateException and "Ensures the truth of an expression involving the state of the calling instance, but not involving any parameters to the calling method." (my emphasis)
Of course, that just expresses the opinions of the authors of Guava. There are others who might argue that IllegalStateException is appropriate here, or neither IllegalStateException nor IllegalArgumentException. It really doesn't matter too much. I can just throw RuntimeException if you do not agree with IllegalArgumentException.

huaxingao

LGTM

wypoon · 2024-11-18T04:43:40Z

@Fokko can you help merge this if you have no further feedback?

flyrain

LGTM. Thanks @wypoon for picking it up!

flyrain · 2024-11-19T07:30:50Z

Hi @Fokko, do you have any further feedback?

huaxingao · 2024-11-20T19:56:57Z

@Fokko @flyrain Should we merge this PR? I am waiting for it to be merged so I can clean up my temporary code

flyrain · 2024-11-20T20:00:22Z

I will merge it if there is no new comment by EOD.

szehon-ho

Very small comment, lgtm otherwise

szehon-ho · 2024-11-20T22:39:07Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReader.java

@@ -28,5 +28,14 @@ public interface ParquetValueReader<T> {

  List<TripleIterator<?>> columns();

+  /**
+   * @deprecated since 1.8.0, will be removed in 1.9.0; use setPageSource(PageReadStore) instead.


Nit: typically we use a link?

flyrain · 2024-11-21T01:47:18Z

Thanks @wypoon for working on it. Thanks @huaxingao @Fokko @szehon-ho for the review.

…it (apache#11520)

There are two Iceberg PRs that "broke" NesQuEIT: * apache/iceberg#11478 caused `testRewriteManifests` to fail due to the changed outcome of the `rewrite_manifests` procedure * apache/iceberg#11520 caused a class-path issue w/ Scala 2.13

Workaround for apache/iceberg#11520 that caused a class-path issue w/ Scala 2.13

There are two Iceberg PRs that "broke" NesQuEIT: * apache/iceberg#11478 caused `testRewriteManifests` to fail due to the changed outcome of the `rewrite_manifests` procedure * apache/iceberg#11520 caused a class-path issue w/ Scala 2.13

Workaround for apache/iceberg#11520 that caused a class-path issue w/ Scala 2.13

flyrain and others added 5 commits November 11, 2024 16:19

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

f435ca1

Change ColumnarBatchReader for Spark 3.4 and 3.5

ffa7e61

Retain old methods but deprecate them, to avoid breaking the API.

832ee7a

Fix inadvertent renaming of a parameter.

73d1380

Fix inadvertent dropping of a final method modifier.

00a4b3c

github-actions bot added spark parquet arrow labels Nov 12, 2024

wypoon mentioned this pull request Nov 12, 2024

Parquet: Use native getRowIndexOffset support instead of calculating it #10107

Closed

wypoon changed the title ~~Parquet use get row index offset~~ Parquet: Use native getRowIndexOffset support instead of calculating it Nov 12, 2024

Fokko reviewed Nov 12, 2024

View reviewed changes

Update deprecation comments.

ff29af6

huaxingao reviewed Nov 13, 2024

View reviewed changes

Throw an exception if PageReadStore does not have the row index offset.

d189412

huaxingao reviewed Nov 15, 2024

View reviewed changes

huaxingao approved these changes Nov 15, 2024

View reviewed changes

flyrain approved these changes Nov 18, 2024

View reviewed changes

szehon-ho reviewed Nov 20, 2024

View reviewed changes

szehon-ho approved these changes Nov 21, 2024

View reviewed changes

Address nit.

661b264

wypoon force-pushed the parquet_use_getRowIndexOffset branch from 0a7f821 to 661b264 Compare November 21, 2024 00:25

flyrain merged commit 93a0633 into apache:main Nov 21, 2024
49 checks passed

huaxingao mentioned this pull request Nov 21, 2024

Spark: remove ROW_POSITION from project schema #11610

Merged

nastra pushed a commit to nastra/iceberg that referenced this pull request Nov 21, 2024

Parquet: Use native getRowIndexOffset support instead of calculating …

02ed7ef

…it (apache#11520)

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

Parquet: Use native getRowIndexOffset support instead of calculating …

29fb251

…it (apache#11520)

snazy added a commit to snazy/query-engine-integration-tests that referenced this pull request Jan 10, 2025

Fix NesQuEIT to pass against recent Iceberg changes

7ad9384

Workaround for apache/iceberg#11520 that caused a class-path issue w/ Scala 2.13

This was referenced Jan 10, 2025

Fix NesQuEIT to pass against recent Iceberg changes projectnessie/query-engine-integration-tests#684

Merged

Fix NesQuEIT to pass against recent Iceberg changes projectnessie/nessie#10184

Merged

snazy added a commit to projectnessie/query-engine-integration-tests that referenced this pull request Jan 13, 2025

Fix NesQuEIT to pass against recent Iceberg changes (#684)

e1faefa

Workaround for apache/iceberg#11520 that caused a class-path issue w/ Scala 2.13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parquet: Use native getRowIndexOffset support instead of calculating it #11520

Parquet: Use native getRowIndexOffset support instead of calculating it #11520

wypoon commented Nov 12, 2024

wypoon commented Nov 12, 2024

Fokko Nov 12, 2024

wypoon Nov 12, 2024 •

edited

Loading

Fokko Nov 12, 2024

wypoon commented Nov 12, 2024

huaxingao Nov 13, 2024

wypoon Nov 13, 2024 •

edited

Loading

wypoon commented Nov 14, 2024

huaxingao Nov 15, 2024

wypoon Nov 15, 2024

huaxingao left a comment

wypoon commented Nov 18, 2024

flyrain left a comment

flyrain commented Nov 19, 2024

huaxingao commented Nov 20, 2024

flyrain commented Nov 20, 2024

szehon-ho left a comment

szehon-ho Nov 20, 2024

wypoon Nov 21, 2024

flyrain commented Nov 21, 2024

	* @deprecated since 1.6.0, will be removed in 1.7.0; use setPageSource(PageReadStore) instead.
	* @deprecated since 1.8.0, will be removed in 1.9.0 or 2.0.0; use setPageSource(PageReadStore) instead.

Parquet: Use native getRowIndexOffset support instead of calculating it #11520

Parquet: Use native getRowIndexOffset support instead of calculating it #11520

Conversation

wypoon commented Nov 12, 2024

wypoon commented Nov 12, 2024

Fokko Nov 12, 2024

Choose a reason for hiding this comment

wypoon Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Fokko Nov 12, 2024

Choose a reason for hiding this comment

wypoon commented Nov 12, 2024

huaxingao Nov 13, 2024

Choose a reason for hiding this comment

wypoon Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

wypoon commented Nov 14, 2024

huaxingao Nov 15, 2024

Choose a reason for hiding this comment

wypoon Nov 15, 2024

Choose a reason for hiding this comment

huaxingao left a comment

Choose a reason for hiding this comment

wypoon commented Nov 18, 2024

flyrain left a comment

Choose a reason for hiding this comment

flyrain commented Nov 19, 2024

huaxingao commented Nov 20, 2024

flyrain commented Nov 20, 2024

szehon-ho left a comment

Choose a reason for hiding this comment

szehon-ho Nov 20, 2024

Choose a reason for hiding this comment

wypoon Nov 21, 2024

Choose a reason for hiding this comment

flyrain commented Nov 21, 2024

wypoon Nov 12, 2024 •

edited

Loading

wypoon Nov 13, 2024 •

edited

Loading