Bloom filter config tweaks (#3023) #3175
Conversation
| let bloom_filter = props | ||
| .bloom_filter_properties(descr.path()) | ||
| .map(|props| Sbbf::new_with_ndv_fpp(*ndv, *fpp)) | ||
| .transpose()?; |
There was a problem hiding this comment.
It is better to propagate the error than printing it
| &self, | ||
| col: &ColumnPath, | ||
| ) -> Option<BloomFilterProperties> { | ||
| ) -> Option<&BloomFilterProperties> { |
There was a problem hiding this comment.
Generally it is better to return a reference so that callers can clone only if necessary
| /// discarded as no-op. | ||
| /// bloom filter if not previously enabled. | ||
| /// | ||
| /// # Panics |
There was a problem hiding this comment.
Better IMO to fail loud than silently
| /// Returns the [`BloomFilterProperties`] for the given column | ||
| /// | ||
| /// Returns `None` if bloom filter is disabled | ||
| pub fn bloom_filter_properties( |
There was a problem hiding this comment.
Given this returns BloomFilterProperties and not a boolean, I thought this name was more appropriate
tustvold
force-pushed
the
bloom-filter-config-tweaks
branch
from
24d5527 to
d13e3ec
Compare
|
Benchmark runs are scheduled for baseline = 4b9e3fe and contender = eefbdce. eefbdce is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Part of #3023
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?