Data, Spark 4.2: Add format model TCK coverage for vectorized reads by joyhaldar · Pull Request #17610 · apache/iceberg

joyhaldar · 2026-08-11T17:12:27Z

Adds a TCK subclass for Spark's vectorized ColumnarBatch read path.

TestSparkVectorizedFormatModel extends ReadFormatModelTests and redefines FILE_FORMATS and FORMAT_AND_GENERATOR to skip Avro, which has no ColumnarBatch model registered. This required making DataGenerator, DataGenerators and DataGenerators.ALL public in the test package.

Two base class changes were needed:

assertRecordsEqual() - read tests built expectations via convertToEngineRecords, which can't work for a batch type
Read assertions moved inside try-with-resources, columnar batches don't survive the reader closing

pvary · 2026-08-12T14:40:09Z

+    return false;
+  }
+
+  protected Set<FileFormat> supportedFormats() {


I don't really like this one.

Could we just re-define FILE_FORMATS, and FORMAT_AND_GENERATOR in TestSparkVectorizedFormatModel?

private static final FileFormat[] FILE_FORMATS = {FileFormat.ORC, FileFormat.PARQUET}; private static final List<Arguments> FORMAT_AND_GENERATOR

This relies on the fact, how junit works internally, but no change is required in all of the tests.

I'm open for other ideas as well, this is just the first one occured to me

Thank you for the suggestion @pvary I tried it out and here's what I found:

Redefining FILE_FORMATS in the subclass works as you described, those tests drop AVRO with no base class changes.

Redefining FORMAT_AND_GENERATOR doesn't work as is, because it references DataGenerators, which is package-private in org.apache.iceberg.data while TestSparkVectorizedFormatModel is in org.apache.iceberg.spark.data.

I tried making DataGenerators, DataGenerators.ALL and the DataGenerator interface public, and with that both fields can be redefined in the subclass. All AVRO cases are gone and I could remove supportedFormats() and the per test assumptions entirely. TestSparkFormatModel and TestFlinkFormatModel still pass.

Would you like me to go with that, or would you rather avoid widening the visibility of those test classes?

I'm fine with making the DataGenerator and related stuff public in the test packages

Thank you @pvary.

Made DataGenerator, DataGenerators and DataGenerators.ALL public in the test package.

TestSparkVectorizedFormatModel redefines both FILE_FORMATS and FORMAT_AND_GENERATOR.

Dropped supportedFormats() and all the per test assumeSupportedFormat() calls from the base class.

One thing I hit while doing this. When the subclass redefines FORMAT_AND_GENERATOR, it also has to repeat every filter the base applies. #17617 added supportsGenerator yesterday, my subclass didn't have it, and ORC + Variant came back and failed CI.

To fix it I'd have to either make supportsGenerator public, or copy the check into the subclass. Neither seems clean, and any new filter will have the same problem IIUC.

protected is enough

pvary · 2026-08-12T14:53:08Z

    return false;
  }

+  protected boolean readOnly() {


Shall we split the base test class to read-tests, and read+write-tests?
Using this method in all test seem awkward a bit.

If we decide so, we might want to do it in a different PR

I agree with you, would love your advice.

Two ways we could do it:

Keep the read tests and shared helpers in BaseFormatModelTests, move the write tests into a BaseReadWriteFormatModelTests that extends it. Existing tests move to the new class.

Insert a BaseReadFormatModelTests above the current BaseFormatModelTests and put the read tests there. No existing subclass changes its extends.

And TestSparkVectorizedFormatModel would only extend the read-only class.

Would you like to do it in a follow-up PR or this PR?

Please separate out the test to:

ReadFormatModelTests as a base, and move the read tests there

BaseFormatModelTests inheriting from ReadFormatModelTests

I would do this way

This should be in a PR before this change

pvary · 2026-08-17T10:15:18Z

+import org.apache.spark.sql.vectorized.ColumnarBatch;
+import org.junit.jupiter.params.provider.Arguments;
+
+public class TestSparkVectorizedFormatModel extends BaseFormatModelTests<ColumnarBatch> {


package private

pvary · 2026-08-28T08:48:41Z

  @ParameterizedTest
  @FieldSource("FILE_FORMATS")
  void testNestedDefaultValue(FileFormat fileFormat) throws IOException {
+    assumeFalse(supportsBatchReads(), "Vectorized reads do not support nested types");


could we handle this with unsupportedTypeIds?

I added assumeSupportedTypes() to skip tests when the schema contains a type listed in unsupportedTypeIds().

pvary · 2026-08-28T08:50:53Z

  @FieldSource("FILE_FORMATS")
  void testReaderBuilderSplit(FileFormat fileFormat) throws IOException {

+    assumeFalse(supportsBatchReads(), "Batch reads return batches, not rows");


Do we need to override this test, or can we test differently?

I added a countRows() hook that sums the row counts across ColumnarBatch instances and uses .size() for row readers. This allows the shared tests to work for both.

pvary · 2026-08-28T08:51:23Z

  @FieldSource("FILE_FORMATS")
  void testReaderBuilderReuseContainers(FileFormat fileFormat) throws IOException {
-
+    assumeFalse(supportsBatchReads(), "Batch reads reuse differently");


Override the test?

Neither vectorized reader returns the same object twice. ColumnarBatchReader.read always builds a new ColumnarBatch and uses reuse only to decide whether to close the previous vectors IIUC:

if (reuse == null) { closeVectors(); } return new ColumnBatchLoader(numRowsToRead).loadDataToColumnBatch();

VectorizedSparkOrcReaders.read doesn't take a reuse argument at all.

I checked with and without reuseContainers() on both formats and each call returns a new object either way. Is that expected for batch reads? Please correct me if I am wrong.

What about the underlying data structures? I expect that the Arrow Vlvectors should be reused.

The reuse logic is in VectorizedArrowReader.java.

https://github.com/apache/iceberg/blob/main/arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorizedArrowReader.java#L140-L156

In ColumnarBatchReader.java, the previous VectorHolder is passed back to the reader, allowing the underlying Arrow vector to be reused.

iceberg/spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/data/vectorized/ColumnarBatchReader.java

Line 81 in 524fe9e

vectorHolders[i] = readers[i].read(vectorHolders[i], batchSize);

I am getting each vector in a batch like this,

private ValueVector arrowVector(ColumnarBatch batch, int col) { IcebergArrowColumnVector vector = (IcebergArrowColumnVector) batch.column(col); return vector.vectorAccessor().getVector(); }

I have updated the shared test to compare the underlying Arrow vectors for every column, both with and without reuseContainers().

pvary · 2026-08-28T08:57:58Z

  @FieldSource("FILE_FORMATS")
  void testReadMetadataColumnIsDeleted(FileFormat fileFormat) throws IOException {
-
+    assumeFalse(supportsBatchReads(), "DeletedColumnVector requires a delete filter");


Please provide a better message. DeletedColumnVector is Spark specific.
What is the exact issue here? The batch readers doesn't read the DVs either, so checking false seems ok.

It doesn't return false, it throws:

TestSparkVectorizedFormatModel > testReadMetadataColumnIsDeleted(FileFormat) > [2] PARQUET FAILED java.lang.NullPointerException: Cannot load from byte/boolean array because "this.isDeleted" is null at org.apache.iceberg.spark.data.vectorized.DeletedColumnVector.getBoolean(DeletedColumnVector.java:61) at org.apache.spark.sql.vectorized.ColumnarBatchRow.getBoolean(ColumnarBatchRow.java:108) at org.apache.spark.sql.vectorized.ColumnarBatchRow.get(ColumnarBatchRow.java:181) at org.apache.iceberg.spark.data.TestHelpers.assertEquals(TestHelpers.java:772) at org.apache.iceberg.spark.data.TestHelpers.assertEquals(TestHelpers.java:734) at org.apache.iceberg.spark.data.TestHelpers.assertEquals(TestHelpers.java:719) at org.apache.iceberg.spark.data.TestSparkVectorizedFormatModel.assertRecordsEqual(TestSparkVectorizedFormatModel.java:102)

The array is only filled when a delete filter is set IIUC. ORC passes, so this is Parquet only if I am not wrong. I will fix the message. Should I file an issue for this, or send a fix PR? Please correct me if I am wrong.

Yeah, the array is only created by the delete filter.
Let me think what could we do here.

Taking a look at supportsBatchReads - it always return false.

I think we should override this method.

Also we should update testReaderBuilderRecordsPerBatchNotSupported to have a batch and a non-bach version. Maybe with override and rename to testReaderBuilderRecordsPerBatch

I updated the skip message to explain the issue and limited the skip to vectorized Parquet. ORC still runs the test.

I updated testReaderBuilderRecordsPerBatch to test both row and batch readers. Row readers check that recordsPerBatch() throws an exception, while batch readers check the batch size and total row count.

pvary · 2026-08-28T08:58:31Z

  @FieldSource("FILE_FORMATS")
  void testReadMetadataColumnRowLineage(FileFormat fileFormat) throws IOException {
-
+    // TODO: ORC's vectorized reader treats _row_id as a plain constant


Shorten and leave an issue id in the text

I have left issue ID #18543

pvary · 2026-08-28T08:59:18Z

  @ParameterizedTest
  @FieldSource("FILE_FORMATS")
  void testSchemaEvolutionEmptyProjection(FileFormat fileFormat) throws IOException {
+    assumeFalse(supportsBatchReads(), "Row count assertion does not apply to batches");


I added a countRows() hook that sums the row counts across ColumnarBatch instances and uses .size() for row readers. This allows the shared tests to work for both.

github-actions · 2026-09-29T00:27:55Z

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.

pvary · 2026-09-29T12:03:43Z

@joyhaldar: Are you planning to work on this? Shall we take over? Maybe @Guosmilesmile could help, if you want.

joyhaldar · 2026-09-29T12:23:18Z

@joyhaldar: Are you planning to work on this? Shall we take over? Maybe @Guosmilesmile could help, if you want.

Sorry for the delay, I was still planning to work on this, but I've been consumed by other priorities. I would still require some time to get back to this. I would not mind if you'd like to take it over, but I am planning to make time in the coming weeks.

rshdhere · 2026-10-14T21:34:38Z

@joyhaldar: Are you planning to work on this? Shall we take over? Maybe @Guosmilesmile could help, if you want.

Sorry for the delay, I was still planning to work on this, but I've been consumed by other priorities. I would still require some time to get back to this. I would not mind if you'd like to take it over, but I am planning to make time in the coming weeks.

Hi @joyhaldar, if you're still short on time I'd be happy to pick this up. I'd keep your commits and address @pvary's comments (splitting testReaderBuilderRecordsPerBatchNotSupported into batch and non-batch versions, and checking reuse of the underlying Arrow vectors). If you'd rather finish it yourself, no problem at all, I'll leave it with you.

Co-authored-by: Joy Haldar <joy.haldar@target.com>

joyhaldar · 2026-10-16T04:37:07Z

@joyhaldar: Are you planning to work on this? Shall we take over? Maybe @Guosmilesmile could help, if you want.

Sorry for the delay, I was still planning to work on this, but I've been consumed by other priorities. I would still require some time to get back to this. I would not mind if you'd like to take it over, but I am planning to make time in the coming weeks.

Hi @joyhaldar, if you're still short on time I'd be happy to pick this up. I'd keep your commits and address @pvary's comments (splitting testReaderBuilderRecordsPerBatchNotSupported into batch and non-batch versions, and checking reuse of the underlying Arrow vectors). If you'd rather finish it yourself, no problem at all, I'll leave it with you.

Hello, sorry for the delay, I am working this over the weekend.

…o 4.2 Co-authored-by: Joy Haldar <joy.haldar@target.com>

… for batch readers Co-authored-by: Joy Haldar <joy.haldar@target.com>

Co-authored-by: Joy Haldar <joy.haldar@target.com>

pvary · 2026-10-20T12:00:51Z

      readRecords = ImmutableList.copyOf(reader);
    }

    assertThat(readRecords).hasSizeGreaterThan(0).hasSizeLessThan(genericRecords.size());


Please check this size checks. Are they still valid for the batches?

pvary · 2026-10-20T12:02:44Z

+  }
+
+  private ValueVector arrowVector(ColumnarBatch batch, int col) {
+    IcebergArrowColumnVector vector = (IcebergArrowColumnVector) batch.column(col);


Are we sure about this? At least an instanceof please

pvary · 2026-10-20T12:04:10Z

  }

+  protected void assumeSupportedTypes(Schema schema) {
+    Collection<Type.TypeID> unsupportedTypeIds = unsupportedTypeIds();


MAybe update the javadoc for unsupportedTypeIds?

github-actions Bot added spark data labels Aug 11, 2026

joyhaldar changed the title ~~[WIP] Data, Spark: Add format model TCK coverage for vectorized reads~~ Data, Spark: Add format model TCK coverage for vectorized reads (WIP) Aug 11, 2026

pvary reviewed Aug 12, 2026

View reviewed changes

joyhaldar mentioned this pull request Aug 13, 2026

Spark 4.1: Fix row lineage in vectorized ORC reads #17635

Closed

joyhaldar force-pushed the spark-vectorized-formatmodel-tck branch 3 times, most recently from 159b391 to 6b66972 Compare August 17, 2026 10:12

pvary reviewed Aug 17, 2026

View reviewed changes

joyhaldar force-pushed the spark-vectorized-formatmodel-tck branch from 6b66972 to 50d5209 Compare August 17, 2026 10:20

joyhaldar force-pushed the spark-vectorized-formatmodel-tck branch from 50d5209 to 625fe88 Compare August 27, 2026 18:28

joyhaldar changed the title ~~Data, Spark: Add format model TCK coverage for vectorized reads (WIP)~~ Data, Spark: Add format model TCK coverage for vectorized reads Aug 28, 2026

joyhaldar marked this pull request as ready for review August 28, 2026 05:47

joyhaldar changed the title ~~Data, Spark: Add format model TCK coverage for vectorized reads~~ Data, Spark 4.1: Add format model TCK coverage for vectorized reads Aug 28, 2026

pvary reviewed Aug 28, 2026

View reviewed changes

github-actions Bot added the stale label Sep 29, 2026

github-actions Bot removed the stale label Sep 30, 2026

Spark: Add format model TCK coverage for vectorized reads

1e95954

Co-authored-by: Joy Haldar <joy.haldar@target.com>

Spark 4.2: skip nested type tests via unsupportedTypeIds, move test t…

e8b97ba

…o 4.2 Co-authored-by: Joy Haldar <joy.haldar@target.com>

joyhaldar and others added 2 commits October 16, 2026 22:43

Count rows via countRows hook so split and empty projection tests run…

2240f75

… for batch readers Co-authored-by: Joy Haldar <joy.haldar@target.com>

Test recordsPerBatch for both batch and row readers

d2cae98

Co-authored-by: Joy Haldar <joy.haldar@target.com>

joyhaldar mentioned this pull request Oct 16, 2026

Spark: vectorized ORC reader ignores row lineage for _row_id and _last_updated_sequence_number #18543

Open

1 of 3 tasks

joyhaldar and others added 2 commits October 16, 2026 23:30

Skip row lineage tests only for vectorized ORC, reference apache#18543

d01b937

Co-authored-by: Joy Haldar <joy.haldar@target.com>

Reword _deleted skip message and limit it to vectorized Parquet

556ce35

Co-authored-by: Joy Haldar <joy.haldar@target.com>

joyhaldar force-pushed the spark-vectorized-formatmodel-tck branch from 625fe88 to 556ce35 Compare October 16, 2026 18:16

joyhaldar changed the title ~~Data, Spark 4.1: Add format model TCK coverage for vectorized reads~~ Data, Spark 4.2: Add format model TCK coverage for vectorized reads Oct 16, 2026

joyhaldar and others added 3 commits October 18, 2026 09:21

Test container reuse for Spark vectorized Parquet reads

0f643c1

Co-authored-by: Joy Haldar <joy.haldar@target.com>

Merge upstream/main into spark-vectorized-formatmodel-tck

96db61e

Co-authored-by: Joy Haldar <joy.haldar@target.com>

Skip unsupported geospatial types in Spark vectorized format tests

04bc928

Co-authored-by: Joy Haldar <joy.haldar@target.com>

pvary reviewed Oct 20, 2026

View reviewed changes

Conversation

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

Uh oh!

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!

pvary Aug 17, 2026 • 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!

joyhaldar Oct 18, 2026 • 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!

pvary Aug 28, 2026 • 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!

github-actions Bot commented Sep 29, 2026

Uh oh!

pvary commented Sep 29, 2026

Uh oh!

joyhaldar commented Sep 29, 2026

Uh oh!

rshdhere commented Oct 14, 2026

Uh oh!

joyhaldar commented Oct 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyhaldar commented Aug 11, 2026 •

edited

Loading

pvary Aug 17, 2026 •

edited

Loading

joyhaldar Oct 18, 2026 •

edited

Loading

pvary Aug 28, 2026 •

edited

Loading