zeroshade commented on code in PR #1135:
URL: https://github.com/apache/arrow-go/pull/1135#discussion_r3752572228
##########
arrow/compute/vector_selection_test.go:
##########
@@ -2366,6 +2366,29 @@ func BenchmarkTakeStringPartitionPattern(b *testing.B) {
b.ReportMetric(float64(numRows*b.N)/b.Elapsed().Seconds(), "rows/sec")
}
+func TestFilterRejectsNonArrayLikeFilter(t *testing.T) {
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+ defer mem.AssertSize(t, 0)
+
+ values, _, err := array.FromJSON(mem, arrow.PrimitiveTypes.Int32,
strings.NewReader("[1]"))
+ require.NoError(t, err)
+ defer values.Release()
+
+ valuesDatum := compute.NewDatum(values)
+ defer valuesDatum.Release()
+ filterRecord := array.NewRecordBatch(
+ arrow.NewSchema([]arrow.Field{{Name: "filter", Type:
arrow.PrimitiveTypes.Int32}}, nil),
+ []arrow.Array{values},
+ 1,
+ )
+ defer filterRecord.Release()
+ filterDatum := compute.NewDatum(filterRecord)
+ defer filterDatum.Release()
+
+ _, err = compute.Filter(context.Background(), valuesDatum, filterDatum,
compute.FilterOptions{})
+ require.ErrorIs(t, err, arrow.ErrNotImplemented)
Review Comment:
Two small coverage gaps.
`TableDatum` is the other kind that isn't array-like, and it's the one that
was previously reaching — or rather, failing to reach — `FilterTable`'s own
guard. One more case would cover the full surface of this check.
Nothing pins the scalar-filter path either. The description says scalar
boolean filters stay supported and that's true today, but it rests on
`ScalarDatum` implicitly satisfying `ArrayLikeDatum`. If that interface or
`ScalarDatum` were refactored, this guard would start rejecting scalar filters
and no test here would notice. A one-line scalar case would make the dependency
explicit — worth checking whether existing tests elsewhere already cover it
before adding.
##########
arrow/compute/selection.go:
##########
@@ -40,7 +40,12 @@ are handled based on FilterOptions.`,
}
filterMetaFunc = NewMetaFunction("filter", Binary(), filterDoc,
func(ctx context.Context, opts FunctionOptions, args ...Datum)
(Datum, error) {
- if args[1].(ArrayLikeDatum).Type().ID() != arrow.BOOL {
+ filter, ok := args[1].(ArrayLikeDatum)
+ if !ok {
+ return nil, fmt.Errorf("%w: filter should be
array-like", arrow.ErrNotImplemented)
Review Comment:
Worth noting for anyone reading this later: the message and sentinel here
deliberately match the guard that already exists in `FilterTable` (line 752),
which has the identical `default:` case returning `"%w: filter should be
array-like"` with `arrow.ErrNotImplemented`.
That's the more interesting framing of this fix. `FilterTable` was *already*
handling non-array-like filters correctly — but for the `KindTable` path,
`filterMetaFunc` dereferenced `args[1].(ArrayLikeDatum).Type()` at the top and
panicked before control ever reached it. So this doesn't just add a guard, it
makes an existing correct guard reachable.
I'd started to suggest rewording this to parallel the `"filter argument must
be boolean type"` line below it — matching the existing `FilterTable` message
is the better choice, so ignore that thought.
--
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]
