Skip to content

Fix variant type filtering in ParquetMetricsRowGroupFilter #14081

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
amogh-jahagirdar merged 9 commits into from
Sep 20, 2025
Merged

Fix variant type filtering in ParquetMetricsRowGroupFilter #14081

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
83ae077
Fix variant type filtering in ParquetMetricsRowGroupFilter
geruh Sep 15, 2025
1d41313
Address comments and add more tests
geruh Sep 16, 2025
99dc168
Refactor tests to be format specific
geruh Sep 16, 2025
ab202af
Revert refactoring
geruh Sep 17, 2025
c7c715e
Revert the test refactoring and clean up original
geruh Sep 18, 2025
3f06a3d
remove old tests
geruh Sep 18, 2025
3ba38bb
clean up and add another test
geruh Sep 19, 2025
a618461
checkstyle..
geruh Sep 19, 2025
a48ec72
address comments
geruh Sep 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 96 additions & 0 deletions data/src/test/java/org/apache/iceberg/data/TestMetricsRowGroupFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import org.apache.iceberg.avro.AvroSchemaUtil;
import org.apache.iceberg.data.orc.GenericOrcReader;
import org.apache.iceberg.data.orc.GenericOrcWriter;
import org.apache.iceberg.data.parquet.GenericParquetWriter;
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.expressions.Expression;
import org.apache.iceberg.io.CloseableIterable;
Expand All @@ -78,6 +79,10 @@
import org.apache.iceberg.types.Types.FloatType;
import org.apache.iceberg.types.Types.IntegerType;
import org.apache.iceberg.types.Types.StringType;
import org.apache.iceberg.variants.ShreddedObject;
import org.apache.iceberg.variants.Variant;
import org.apache.iceberg.variants.VariantMetadata;
import org.apache.iceberg.variants.Variants;
import org.apache.orc.OrcFile;
import org.apache.orc.Reader;
import org.apache.parquet.hadoop.ParquetFileReader;
Expand Down Expand Up @@ -988,6 +993,97 @@ public void testTransformFilter() {
.isTrue();
}

@TestTemplate
public void testVariantFilterNotNull() throws IOException {
assumeThat(format).isEqualTo(FileFormat.PARQUET);

Schema variantSchema =
new Schema(
required(1, "id", IntegerType.get()),
optional(2, "variant_field", Types.VariantType.get()));

File parquetFile = new File(tempDir, "test-variant" + System.nanoTime());

OutputFile outFile = Files.localOutput(parquetFile);
try (FileAppender<GenericRecord> appender =
Parquet.write(outFile)
.schema(variantSchema)
.createWriterFunc(GenericParquetWriter::create)
.build()) {

for (int i = 0; i < 10; i++) {
GenericRecord record = GenericRecord.create(variantSchema);
record.setField("id", i);

if (i % 2 == 0) {
VariantMetadata metadata = Variants.metadata("field");
ShreddedObject obj = Variants.object(metadata);
obj.put("field", Variants.of("value" + i));
Variant variant = Variant.of(metadata, obj);
record.setField("variant_field", variant);
}

Comment thread
amogh-jahagirdar marked this conversation as resolved.
Outdated
appender.add(record);
}
}

@amogh-jahagirdar amogh-jahagirdar Sep 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to write records in these tests? At this level of abstraction, I think we should just create the row group filter with the notNull("variant_field") and it assert that shouldRead is true.

@aihuaxu aihuaxu Sep 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Seems we don't need to write to the files.

@geruh geruh Sep 16, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback Amogh!

Good point about avoiding file writing at this level. I initially wrote the tests this way because this test class shares a schema for writing out data files in both ORC and Parquet. We're now in a situation where ORC doesn't have full support for variant types while Parquet does, so adding variant fields to the shared schema would break the existing ORC tests.

That said, it probably makes sense to use separate schemas for Parquet and ORC given the differnt levels of support. and write the tests to reflect that.

@amogh-jahagirdar amogh-jahagirdar Sep 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked with @geruh offline and I also poked around refactoring this class, and while I think we should (this mixture of orc/parquet both trying to test "row group filtering" is leading to weird tests), it's a big change especially for something going into a patch release.

Also while technically the implementation of filtering with variant doesn't depend on the actual contents of the file, after some more thought I concluded that it's better to write a more realistic test which does contain the records like @geruh was doing before.


InputFile inFile = Files.localInput(parquetFile);
try (ParquetFileReader reader = ParquetFileReader.open(parquetInputFile(inFile))) {
assertThat(reader.getRowGroups()).as("Should create only one row group").hasSize(1);
BlockMetaData blockMetaData = reader.getRowGroups().get(0);
MessageType fileSchema = reader.getFileMetaData().getSchema();
Comment thread
geruh marked this conversation as resolved.

ParquetMetricsRowGroupFilter rowGroupFilter =
new ParquetMetricsRowGroupFilter(variantSchema, notNull("variant_field"), true);
boolean shouldRead = rowGroupFilter.shouldRead(fileSchema, blockMetaData);
assertThat(shouldRead)
Comment thread
geruh marked this conversation as resolved.
Outdated
.as("Should read: variant notNull filters must be evaluated post scan")
.isTrue();
}
parquetFile.deleteOnExit();
}

@TestTemplate
public void testAllNullsVariantNotNull() throws IOException {
assumeThat(format).isEqualTo(FileFormat.PARQUET);

Schema variantSchema =
new Schema(
required(1, "id", IntegerType.get()),
optional(2, "variant_field", Types.VariantType.get()));

File parquetFile = new File(tempDir, "test-variant-nulls" + System.nanoTime());

OutputFile outFile = Files.localOutput(parquetFile);
try (FileAppender<GenericRecord> appender =
Parquet.write(outFile)
.schema(variantSchema)
.createWriterFunc(GenericParquetWriter::create)
.build()) {

for (int i = 0; i < 10; i++) {
GenericRecord record = GenericRecord.create(variantSchema);
record.setField("id", i);
record.setField("variant_field", null);
appender.add(record);
}
}

@amogh-jahagirdar amogh-jahagirdar Sep 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, don't think we need to actually write parquet files in these tests

@geruh geruh Sep 16, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed above

InputFile inFile = Files.localInput(parquetFile);
try (ParquetFileReader reader = ParquetFileReader.open(parquetInputFile(inFile))) {
BlockMetaData blockMetaData = reader.getRowGroups().get(0);
MessageType fileSchema = reader.getFileMetaData().getSchema();

ParquetMetricsRowGroupFilter rowGroupFilter =
new ParquetMetricsRowGroupFilter(variantSchema, notNull("variant_field"), true);
boolean shouldRead = rowGroupFilter.shouldRead(fileSchema, blockMetaData);
assertThat(shouldRead)
Comment thread
geruh marked this conversation as resolved.
Outdated
.as("Should read: variant notNull filters must be evaluated post scan even for all nulls")
.isTrue();
}
parquetFile.deleteOnExit();
}

private boolean shouldRead(Expression expression) {
return shouldRead(expression, true);
}
Expand Down
6 changes: 4 additions & 2 deletions parquet/src/main/java/org/apache/iceberg/parquet/ParquetMetricsRowGroupFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,10 @@ public <T> Boolean notNull(BoundReference<T> ref) {

// When filtering nested types notNull() is implicit filter passed even though complex
// filters aren't pushed down in Parquet. Leave all nested column type filters to be
// evaluated post scan.
if (schema.findType(id) instanceof Type.NestedType) {
// evaluated post scan. Variant types also need to be evaluated post scan to access
// shredded statistics.

@amogh-jahagirdar amogh-jahagirdar Sep 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove the "variant types also need to be evaluated post scan to access shredded statistics". I'd just update to reflect the current state of things which is in that first sentence, "When filtering nested types or variant...." and the second sentence to be "Leave these type filters...".

For shredded stats pruning, the core library already contains BoundExtract, what we need is the translation/plumbing from engines to that extract, and then I think we can do pruning based on the shredded stats. For now though , I'd just leave it out of comments since I think it's more confusing and a bit inaccurate.

@aihuaxu aihuaxu Sep 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on this.

Type type = schema.findType(id);
if (type instanceof Type.NestedType || type.isVariantType()) {

@RussellSpitzer RussellSpitzer Sep 15, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

return ROWS_MIGHT_MATCH;
}

Expand Down