zeroshade commented on code in PR #1033:
URL: https://github.com/apache/arrow-go/pull/1033#discussion_r3666628168
##########
parquet/writer_properties.go:
##########
@@ -366,6 +371,9 @@ func WithPageIndexEnabledPath(path ColumnPath, enabled
bool) WriterProperty {
// it is abandoned and not written to the file.
func WithMaxBloomFilterBytes(nbytes int64) WriterProperty {
return func(cfg *writerPropConfig) {
+ if nbytes < minimumBloomFilterBytes || nbytes >
maximumBloomFilterBytes {
+ panic("parquet: maximum bloom filter size must be
between 32 bytes and 128 MiB")
+ }
Review Comment:
This is the item I'd most like addressed before merge, because it changes
documented public behavior.
The **lower** bound is a strict improvement — values below 32 already
crashed at write time (`InsertHash` on a zero-byte filter), so panicking early
is better in every way.
The **upper** bound is different: `WithMaxBloomFilterBytes(nbytes > 128
MiB)` previously *worked*. It was silently clamped downstream by
`min(maximumBloomFilterBytes, maxBytes)`, so a caller passing e.g. 256 MiB got
a valid 128 MiB filter and a working writer. After this change that same call
panics.
Failing fast is arguably the right choice, and it's the consistent
resolution of the policy question I raised. But the doc comment just above
(`:369-371`) doesn't mention that this setter can panic at all, so a caller has
no way to know. Could you add a line — something like *"Panics if nbytes is
outside the supported range of 32 bytes to 128 MiB."*
@zeroshade may also want a release-note entry, since this converts
previously-working input into a panic.
Minor wording point: the panic message hard-codes "32 bytes and 128 MiB"
while the bounds come from `minimumBloomFilterBytes`/`maximumBloomFilterBytes`.
If those constants ever change, the message goes stale — worth formatting them
in.
##########
parquet/writer_properties.go:
##########
@@ -404,10 +413,17 @@ func WithBloomFilterFPP(fpp float64) WriterProperty {
// for writing bloom filters.
func WithBloomFilterFPPFor(path string, fpp float64) WriterProperty {
return func(cfg *writerPropConfig) {
+ validateBloomFilterFPP(fpp)
cfg.bloomFilterFPPs[path] = fpp
}
}
+func validateBloomFilterFPP(fpp float64) {
+ if fpp <= 0 || fpp >= 1 || math.IsNaN(fpp) {
+ panic("parquet: bloom filter false-positive probability must be
in (0, 1)")
+ }
+}
Review Comment:
No change needed here — the helper and its placement are right, and routing
`WithBloomFilterFPP`, `WithBloomFilterFPPFor` and (transitively)
`WithBloomFilterFPPPath` through it is what makes the deeper panic unreachable
from the public API. I verified there's no bypass:
`defColumnProps`/`columnProps` are unexported and no `WriterProperty` injects a
whole `ColumnProperties`.
One note for the future, since this file now needs the bounds:
`minimumBloomFilterBytes`/`maximumBloomFilterBytes` exist in **both** `parquet`
(`:64-65`) and `parquet/metadata` (`bloom_filter.go:44,46`). I confirmed the
duplication is forced — `parquet/metadata` imports `parquet`, so the reverse
direction would be an import cycle — and the values agree today (32 /
134217728). Still a silent-drift hazard if one side is ever tuned. A short
comment on each pair pointing at the other would make that safe.
##########
parquet/metadata/bloom_filter_test.go:
##########
@@ -272,6 +287,27 @@ func TestAdaptiveBloomFilterEdgeCases(t *testing.T) {
assert.Truef(t, bf.CheckHash(h), "hash %d not found
after GC - potential GC safety issue", h)
}
})
+
+ t.Run("clamps maximum size to the minimum allocation", func(t
*testing.T) {
+ bf := NewAdaptiveBlockSplitBloomFilter(0, 1, 0.01, col,
mem).(*adaptiveBlockSplitBloomFilter)
+ defer func() {
+ for _, candidate := range bf.candidates {
+ candidate.bloomFilter.cancelCleanup()
+ candidate.bloomFilter.data.Release()
+ }
+ }()
+
+ assert.EqualValues(t, minimumBloomFilterBytes, bf.maxBytes)
+ assert.NotPanics(t, func() { bf.InsertHash(1) })
+ })
+
+ t.Run("rejects invalid false-positive probabilities", func(t
*testing.T) {
+ for _, fpp := range []float64{-0.1, 0, 1, math.NaN()} {
+ assert.PanicsWithValue(t,
+ "parquet: bloom filter false-positive
probability must be in (0, 1)",
+ func() { NewAdaptiveBlockSplitBloomFilter(1024,
1, fpp, col, mem) })
Review Comment:
`assert.PanicsWithValue` pins the exact panic string, so any rewording of
the message — including the `parquet:` prefix — breaks this test with no
behavior change. The sibling subtest just above uses `assert.NotPanics`, and
the rest of this file favors the looser forms; `assert.Panics` would match.
If you want to keep asserting *which* validation fired, a substring check on
the recovered value would be more durable than exact equality.
Also worth knowing about this subtest specifically: against **unpatched**
code it wouldn't fail cleanly, it would **hang** (that's the infinite loop in
`expectedNDV` this PR fixes), so on the pre-fix tree it manifests as a CI
timeout rather than a crisp assertion failure. That's fine for guarding the
patched behavior — just noting it isn't a sharp regression detector if anyone
ever reverts the production change.
--
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]
