hcrosse opened a new pull request, #1457: URL: https://github.com/apache/arrow-go/pull/1457
### Rationale for this change `is_null`, `is_not_null` and `is_nan` can panic because their dispatch assumes two arguments. For example, `is_null` on any decimal array panics. In iceberg-go, a decimal `IsNull`, `NotNull` or `IN` filter crashes the process. ### What changes are included in this PR? The three functions are now plain scalar functions with exact dispatch, as in Arrow C++. They were registered as `compareFunction`, whose `DispatchBest` uses helpers that read a second argument. A short comment on `compareFunction` records that assumption. `is_not_null` already returns wrong results on sliced input, for example on sliced int32 arrays (https://github.com/apache/arrow-go/issues/1453). Decimals panicked before reaching that bug. With this change alone, `is_not_null` on a sliced decimal array returns a wrong result instead of panicking. https://github.com/apache/arrow-go/pull/1456 fixes the offset bug and should land before or with this PR. ### Are these changes tested? I added `TestDecimal` for `is_null`, `is_not_null` and `is_nan` on `decimal128` and `decimal256`, and `TestIsNaNDictionaryNotImplemented` for a float dictionary returning `arrow.ErrNotImplemented`. All seven subtests panic on main and pass with this change. I also compared main with this change on 171 cases, covering 57 input types for each of the three functions, on macOS. Calls that previously panicked now return results or errors. Two timestamp null checks also return results instead of rejecting a time zone that cannot be loaded. Every case that returned a result before returns the same result now. With iceberg-go main `8ef0a092`, decimal `IsNull`, `NotNull`, `IN` and `NOT IN` scan filters return the expected rows where they crashed before. On Linux amd64, `go test -count=1 ./arrow/compute/...`, `go vet`, `gofmt` and `golangci-lint` passed, along with the `prek` hooks on the changed files, including the Release Audit Tool. I did not test arm64. ### Are there any user-facing changes? Yes. `is_null` and `is_not_null` return results on decimal input. `is_nan` on decimal input returns all false through its existing kernel. As before, `is_nan` returns false for null inputs, where Arrow C++ returns null. Inputs without an `is_nan` kernel, including dictionaries, strings and unsupported temporal types, return `arrow.ErrNotImplemented` instead of panicking. I checked the dictionary, string and timestamp cases against Arrow C++. `is_null` and `is_not_null` no longer reject a timestamp whose time zone cannot be loaded, because neither function uses the zone. This also matches Arrow C++. There is no API change. Closes https://github.com/apache/arrow-go/issues/1454. -- 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]
