I don't quite understand this. Can we inject these properties directly when the table is created instead of changing it after it is created?

table = catalog.createTable(TableIdentifier.of("default", name), schema);

These properties can be set either at table creation or updated later. I don't think it matters. The reason I picked the second choice is because when I wrote the test, I happened to use TestSparkReaderDeletes as my template and followed the style there.

+1 to following existing style

It would be better to document that the NDV is specific for a parquet file.

Thank for your comment. I documented this in configuration.md.

does it ever make sense to enable bloom filter for all columns? should we only allow bloom filter for explicitly specified columns?

It is not a common usage to enable bloom filter for all columns, but it's legal. This is consistent with the parquet-mr bloom filter implementations.

I agree with Steven. I think it only makes sense to enable bloom filters for some columns.

+1. While it's consistent with the parquet-mr bloom filter implementaiton, we need to think of user experience first and foremost.

It doesn't make sense to enable bloom filters for a lot of columns. And many users don't do any tuning of their metadata / statistics.

I think it's in-line with other things we do to make the users experience better, like turning off column level statistics after a certain number of columns. We can point it out in the docs under a big !!!NOTE (that's highlighted) that bloom filter is only used when turned on.

It's really an advanced thing to use at all imo.

Sounds good. I have removed DEFAULT_PARQUET_BLOOM_FILTER_ENABLED. Now user needs to enable bloom filter for individual column using PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX. If the column is a complex type, user needs to enable the column inside the complex type, for example,
set(PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX + "struct_col.int_field", "true")

What is the behavior of this? If the NDV requires a size that is too large, does it skip writing the bloom filter?

If the NDV requires a size that is too large, parquet still writes the bloom filter using the max bytes set by this property, not using the bitset calculated by NDV.

I guess there probably isn't much we can do about this, although that behavior makes no sense to me. Is it possible to set the expected false positive probability anywhere? Or is that hard-coded in the Parquet library?

There isn't a property to set fpp in Parquet.

What fpp is used by Parquet?

Parquet uses 0.01 for fpp.
I chatted offline with @chenjunjiedada, we can probably add a config for fpp in parquet first, and then in iceberg.

@aokolnychyi, what do you think about removing the old ParquetWriteAdapter code? I don't think that anyone uses it anymore.

Why not add bloom filter testing to one of the existing cases? I don't see a reason why this can't be tested along with other cases. And Spark tests already take a long time. We should avoid adding more cases.

I want to have some bloom filter write/read tests that involve copy on write or merge on read. I think this is the only place that test copy on write or merge on read (I maybe wrong). That's why I added a parameter here to turn on bloom filter to test.

I think it's fine to have some tests with the row level operations. What I was pointing out is that we can add bloom filters to the existing tests. There's no need to execute the entire suite an additional time.

I see. Thanks for the clarification. Instead of executing the entire suite, is it OK to add a new (but small) test suite to test bloom filter for copy on write or merge on read? I didn't find any existing copy on write/merge on read tests with filters.

I don't think it is a good idea to add a second boolean argument to this method. It is confusing enough with just one.

How about using a different method name for this and then renaming this to a be a private internal implementation?

I changed this method name to exprReferences (please let me know if you have a better name), but I still need to keep this as public because I need to access this method from ParquetBloomRowGroupFilter, which is in a different package.

Maybe just references since it doesn't bind?

Sounds good. Will change.

Since this uses column name in the config, is there any logic to update these configs when columns are renamed?

Good question. I don't have a logic to update the configs when columns are renamed. I think we are OK, though. At write path, I use these configs to write bloom filters at file creation time. I don't use these configs any more for read. At read path, the bloom filters are loaded using id instead of column name. If the columns are renamed after the bloom filters have been written, as long as the id are still the same, the bloom filters should be able to loaded OK.

We're okay for adding read support, but we should consider how to configure write support then.

I think that this needs to use the Parquet type from the file, not the Iceberg type.

The Iceberg type comes from the read schema, which could be long when the file's type is int because of type promotion rules. We need to make sure that the hash function used matches the file's type because that was what produced the bloom filter. Hopefully, the same hash function is used for long and int in Parquet, but I'm not sure that is the case so the safest thing is to convert value to the Parquet type.

I changed the code to switch (primitiveType.getPrimitiveTypeName())

Looks like this will fall through if the type ID is not handled? I think you need to add a break;.

Will fix.

This is what Parquet would do to hash a decimal stored as an int?

Yes