NikitaMatskevich commented on code in PR #5724:
URL: https://github.com/apache/datafusion-comet/pull/5724#discussion_r3997647725
##########
native/core/src/execution/operators/iceberg_write.rs:
##########
@@ -665,8 +666,146 @@ fn build_writer_properties(settings:
&IcebergParquetWriteSettings) -> DFResult<W
.set_dictionary_page_size_limit(settings.dict_size_bytes as usize)
.set_data_page_row_count_limit(settings.page_row_limit as usize)
.set_statistics_enabled(EnabledStatistics::Page)
- .set_statistics_truncate_length(None)
- .build())
+ .set_statistics_truncate_length(None);
+ for column in &settings.bloom_filter_enabled_columns {
+ let path = parquet_column_path(column);
+ let fpp = settings
+ .bloom_filter_fpp_by_column
+ .get(column)
+ .copied()
+ .unwrap_or(ICEBERG_DEFAULT_BLOOM_FILTER_FPP);
+ let ndv = settings.bloom_filter_ndv_by_column.get(column).copied();
+ let max_bytes = settings.bloom_filter_max_bytes as usize;
+ validate_bloom_filter_inputs(fpp, max_bytes)?;
+ let target_bytes = parquet_mr_bloom_filter_bytes(ndv, fpp, max_bytes);
+ let synthetic_ndv = synthetic_ndv_for_bloom_filter_bytes(target_bytes,
fpp)?;
+ builder = builder
+ .set_column_bloom_filter_enabled(path.clone(), true)
+ .set_column_bloom_filter_fpp(path.clone(), fpp)
+ .set_column_bloom_filter_max_ndv(path, synthetic_ndv);
Review Comment:
Thank you for your reviews! I will try to first address one question at a
time:
> Until that exists, could we gate this behind a Comet config that defaults
to the JVM path when any bloom column is enabled, or at minimum make the docs
state plainly that native filters are smaller and have a higher realised FPP
than Java's for sparse row groups?
I totally agree that we should try to contribute a folding on/off toggle
upstream and I volunteer to do so. However, I am not 100% sure we are obliged
to wait until this upstream change is released.
As for me, I think we can introduce a new Comet config param
'iceberg.write.enable-parquet-rs-bf-folding' (or smth like that) which can be
set to 'false' by default and this 'false' branch would disable native writer
for tables with bf. We will document this property and explain the behavior
change behind it in more details. Is this close to what you had in mind
@jordepic ?
As for my company, there are many bloom filters in our iceberg tables, and
full gating (for all tables with BF) would essentially mean that we could not
benefit from Comet acceleration OOTB until new parquet-rs is released + this
code too. We will have to run a fork of Comet. It would also make this whole PR
a dead code. Last but not least, we like the folding feature of parquet-rs:
parquet-java, when params are badly configured, might introduce parquet
metadata bloat which might amplify frequently occuring iceberg small file
problem. At the end of the day, it comes down to "too good FPP" vs "too good
memory footprint" and both are equally good behaviors IMO.
From my perspective as a user, param set covered by the current PR already
allows to tune the bloom filter precisely for my needs, and respects all
contracts between me and iceberg. These rs-written filters might become a cause
of regression only in one case - when I set param values which are way off;
this means I am already operating in a dangerous zone - I rely on existing
behavior which is not promised by any existing API contract.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
