Spark: Encapsulate parquet objects for Comet #13786

parthchandra · 2025-08-11T20:24:29Z

Fixes issues in #13378. This makes all the remaining changes to Iceberg to address issues with Comet/Parquet shading. The corresponding changes to Comet are already merged into Comet 0.10.1
Additionally, the dependency of the iceberg-parquet module has been removed because it created an iceberg-flink dependency on comet-spark. The code is changed to use a reflection based CometBridge class to access the come (Parquet) FileReader.
This PR also removes the commit to enable Comet execution (moving it to a follow up PR: cc @hsiang-c).

This PR makes the changes for Spark 3.4, 3.5, and 4.0

parthchandra · 2025-08-11T20:26:00Z

@huaxingao, @anuragmantri please take a look.

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

hsiang-c · 2025-08-11T21:28:02Z

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

+      }
+      RowGroupReader pages;
+      try {
+        pages = cometReader.readNextRowGroup();


readNextRowGroup might return null and setRowGroupInfo will make use of pages.

In this case, advance is called only by next which is implementing the iterator interface and which explicitly checks for hasNext so readNextRowGroup always returns a not null value.
This could be done better, but I'll leave this as it is because -
i) it is not really clear what the behavior will be if advance silently does not do anything.
ii) the code (and behavior) will be consistent with the implementation in VectorizedParquetReader

hsiang-c · 2025-08-11T21:28:34Z

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

-              caseSensitive,
-              maxRecordsPerBatch);
+          if (isComet) {
+            LOG.info("Comet enabled");


(nit) Maybe Comet vectorized reader enabled.

huaxingao · 2025-08-18T00:30:49Z

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

+
+    boolean isRepeated = primitiveType.getRepetition() == Type.Repetition.REPEATED;
+
+    // ToDo: extract this into a Util method


nit: remove this line?

huaxingao · 2025-08-18T00:30:54Z

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

+    }
+
+    return new ParquetColumnSpec(
+        1, // ToDo: pass in the correct id


Done. Thanks for pointing this out!

shangxinli · 2025-09-26T14:36:58Z

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

+          throw new IllegalArgumentException(
+              "Missing required parameters for DecimalLogicalTypeAnnotation: " + params);
+        }
+        int scale = Integer.parseInt(params.get("scale"));


Should we add validation for the parameter format? for example,
If params.get("scale") returns null we will get NumberFormatException. Another case is non-numeric string.

It makes the code very verbose, but I've added the additional validation.

shangxinli · 2025-09-26T14:39:08Z

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

+          specs.add(spec);
+        }
+
+        fileReader.setRequestedSchemaFromSpecs(specs);


If setRequestedSchemaFromSpecs fails, the FileReader will be closed?

Good catch. In the latest update, this is now accessed via a wrapper class which can now throw additional exceptions. The wrapper is now auto closeable and is explicitly closed in this method.

parthchandra · 2025-09-27T00:03:57Z

@shangxinli thank you for taking a look. I found that there are more changes to get around parquet shading that need to be addressed; let me address your comments along with that.

parthchandra · 2025-10-08T00:14:26Z

@huaxingao, @shangxinli, @hsiang-c this is ready for final review.

parthchandra · 2025-10-08T00:15:38Z

cc @anuragmantri

huaxingao · 2025-10-08T07:02:51Z

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

-              maxRecordsPerBatch);
+          if (isComet) {
+            LOG.info("Comet vectorized reader enabled");
+            return new CometVectorizedParquetReader<>(


@pvary I’ve seen your work on the reader/writer API proposal, but I haven’t kept up with all the details. We’re adding a CometVectorizedParquetReader, does this align with your proposal?

Based on the suggestion on that PR, we will just handle Comet as an internal detail for Parquet. So it will be hidden behind the FileFormat API.

Likely the this.isComet will be driven by a configuration Map for the ReadBuilder, but forget about it now. It is not much of a complexity, and we will be handling it when the time comes.

Thanks for your consideration!

@pvary thanks for verifying this. If any change is needed in future, it will be fairly easy to move code around.
@huaxingao thank you for reviewing; there is also a follow up PR - #14062 is in draft because it requires this PR.

parthchandra · 2025-10-10T21:28:09Z

@huaxingao any comments you would like me to address?

huaxingao · 2025-10-10T22:10:27Z

@parthchandra Apologies for the delay. I’ve skimmed the changes and would like more time for a deeper review. Because we’re adding several classes to the Parquet module, Comet code is no longer confined to the Spark module. Given the broader scope, it would be ideal if we can get one more committer to review and sign off.

parthchandra · 2025-10-10T22:47:00Z

Thanks @huaxingao. Who would you recommend we ask for a second review?
Note that the introduction of Comet classes becomes necessary because of the use of Comet to provide parquet file reading. If we use the Iceberg parquet file reader then we won't need to include any Comet classes in the parquet module. Also, to avoid creating a dependency on Comet, the Comet classes in the parquet module use reflection introducing the CometBridge class among others.

huaxingao · 2025-10-10T23:16:25Z

cc @aokolnychyi @flyrain @szehon-ho @stevenzwu Appreciate it if you could please take a look too. Thanks a lot!

huaxingao · 2025-10-10T23:22:34Z

@parthchandra Do we have Comet customers on Spark 3.4? If not, we don't need the changes on v3.4, because 3.4 has been marked as deprecated and will be removed in next release. Also Comet version has already been updated to 0.10.1. Could you please rebase?

Abstract RowGroup and FileReader interaction apis into a reflection based bridge class Update to comet 0.10.1

parthchandra · 2025-10-11T00:39:58Z

@huaxingao we are still supporting Spark 3.4.0 in Comet so it will be useful if we have the Spark 3.4 changes as well. Is the removal of spark 3.4 part of 1.11.0 or after that?
Also, rebased

huaxingao · 2025-10-11T00:47:35Z

@parthchandra There is a discussion to remove 3.4 in 1.11.0. If we need to support 3.4 in Comet for a longer time, we need to -1 in the discussion thread.

huaxingao · 2025-10-11T04:17:08Z

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

+  }
+
+  private static class FileIterator<T> implements CloseableIterator<T> {
+    // private final ParquetFileReader reader;


nit: remove this

huaxingao · 2025-10-11T04:17:24Z

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

+        ByteBuffer fileAADPrefix) {
+      CometBridge.FileReaderWrapper fileReader = null;
+      try {
+        Object cometOptions = CometBridge.createReadOptions(new Configuration());


I can't remember how the ReadOptions works. Is it correct to have an empty Configuration here?
Do we need to verify the ParquetReadOption we build on Comet side is the same as the one on Iceberg side?

huaxingao · 2025-10-11T04:20:15Z

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

+  }
+
+  /** Creates a CometIOException from a general Exception. */
+  public static CometIOException fromException(Exception cause) {


Seems a couple of the methods are not used. Shall we remove the no usages methods?

huaxingao · 2025-10-11T04:24:40Z

@parthchandra @hsiang-c I remember we run Comet/Iceberg integration test in Comet repo. Is it possible to add a Comet integration test in TestRoundTrip?

github-actions bot added spark parquet build labels Aug 11, 2025

hsiang-c reviewed Aug 11, 2025

View reviewed changes

parquet/src/main/java/org/apache/iceberg/parquet/CometVectorizedParquetReader.java Show resolved Hide resolved

hsiang-c reviewed Aug 11, 2025

View reviewed changes

hsiang-c added a commit to hsiang-c/datafusion-comet that referenced this pull request Aug 12, 2025

Port apache/iceberg#13786

5891249

hsiang-c mentioned this pull request Aug 16, 2025

fix: [iceberg] Switch to OSS Spark and run Iceberg Spark tests in parallel apache/datafusion-comet#1987

Merged

huaxingao reviewed Aug 18, 2025

View reviewed changes

parthchandra mentioned this pull request Sep 12, 2025

feat: support deleted rows in Comet #14062

Draft

parthchandra marked this pull request as ready for review September 16, 2025 23:48

parthchandra marked this pull request as draft September 17, 2025 00:03

shangxinli reviewed Sep 26, 2025

View reviewed changes

hsiang-c mentioned this pull request Sep 26, 2025

Parquet: Close reader in CometVectorizedParquetReader #14194

Draft

parthchandra force-pushed the iceberg-shading-pr branch from 0119c24 to a148a5b Compare October 7, 2025 22:49

parthchandra marked this pull request as ready for review October 8, 2025 00:07

huaxingao reviewed Oct 8, 2025

View reviewed changes

huaxingao and others added 2 commits October 10, 2025 17:37

Spark:3.4 Encapsulate parquet objects for Comet

525b282

Update for changes to Comet apis.

866e41e

Abstract RowGroup and FileReader interaction apis into a reflection based bridge class Update to comet 0.10.1

parthchandra force-pushed the iceberg-shading-pr branch from a148a5b to 866e41e Compare October 11, 2025 00:38

huaxingao reviewed Oct 11, 2025

View reviewed changes


		boolean isRepeated = primitiveType.getRepetition() == Type.Repetition.REPEATED;

		// ToDo: extract this into a Util method

Spark: Encapsulate parquet objects for Comet #13786

Are you sure you want to change the base?

Spark: Encapsulate parquet objects for Comet #13786

Uh oh!

Conversation

parthchandra commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parthchandra commented Aug 11, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hsiang-c Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthchandra commented Sep 27, 2025

Uh oh!

parthchandra commented Oct 8, 2025

Uh oh!

parthchandra commented Oct 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthchandra commented Oct 10, 2025

Uh oh!

huaxingao commented Oct 10, 2025

Uh oh!

parthchandra commented Oct 10, 2025

Uh oh!

huaxingao commented Oct 10, 2025

Uh oh!

huaxingao commented Oct 10, 2025

Uh oh!

parthchandra commented Oct 11, 2025

Uh oh!

huaxingao commented Oct 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huaxingao commented Oct 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

parthchandra commented Aug 11, 2025 •

edited

Loading

hsiang-c Aug 11, 2025 •

edited

Loading