zeroshade commented on code in PR #1033:
URL: https://github.com/apache/arrow-go/pull/1033#discussion_r3660790128
##########
parquet/metadata/adaptive_bloom_filter.go:
##########
@@ -79,9 +80,13 @@ type adaptiveBlockSplitBloomFilter struct {
}
func NewAdaptiveBlockSplitBloomFilter(maxBytes uint32, numCandidates int, fpp
float64, column *schema.Column, mem memory.Allocator) BloomFilterBuilder {
+ if fpp <= 0 || fpp >= 1 || math.IsNaN(fpp) {
+ panic("parquet: bloom filter false-positive probability must be
in (0, 1)")
+ }
Review Comment:
The predicate itself is right, including the non-redundant `IsNaN` term.
Two things though. First, this is an exported constructor returning only
`BloomFilterBuilder`, so panic is the only in-signature option — but that means
an invalid *user property* becomes a panic in the write path at
`column_writer.go:811`. Validating in
`WithBloomFilterFPP`/`WithBloomFilterFPPFor` (`writer_properties.go:397-413`)
instead would make this unreachable from the public API.
Second, the same unguarded `fpp` reaches `NewBloomFilterFromNDVAndFPP`
(`bloom_filter.go:327`) from `column_writer.go:807`, one line away from the
call this protects. That path still hangs on fpp=1 and still panics for small
`maxBytes` after this patch.
##########
parquet/metadata/adaptive_bloom_filter.go:
##########
@@ -79,9 +80,13 @@ type adaptiveBlockSplitBloomFilter struct {
}
func NewAdaptiveBlockSplitBloomFilter(maxBytes uint32, numCandidates int, fpp
float64, column *schema.Column, mem memory.Allocator) BloomFilterBuilder {
+ if fpp <= 0 || fpp >= 1 || math.IsNaN(fpp) {
+ panic("parquet: bloom filter false-positive probability must be
in (0, 1)")
+ }
+ maxBytes = max(minimumBloomFilterBytes, min(maximumBloomFilterBytes,
maxBytes))
Review Comment:
This clamp is correct and I confirmed it's a no-op for every `maxBytes >=
32`, so valid configurations are unaffected — it only rescues the 0..31 range
that previously produced a zero-byte filter and panicked on insert.
The inconsistency is what I'd flag: an out-of-range `fpp` panics three lines
up, while an out-of-range `maxBytes` is silently corrected here. Both are
user-supplied; picking one policy for both would be clearer. Note
`NewBloomFilterFromNDVAndFPP` has no equivalent clamp at all.
--
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]
