Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-134: Add Validity functions #135

Merged
merged 8 commits into from
Sep 25, 2024
Merged

Conversation

zeroshade
Copy link
Member

Adding is_null, is_not_null, and is_nan functions and tests to the Compute implementations for Go.

This also removes the -ffast-math option from the SIMD optimized compiler Makefile so that our comparisons properly handle NaN values (-ffast-math assumes that no values are NaN and removes the checks). is_nan can be easily implemented for float and double values by dispatching to A != A.

Tests are also added for the new functions.

Ultimately the driving force behind this is as these are necessary for implementing scanning reads in iceberg-go

arrow/array/struct.go Show resolved Hide resolved
arrow/compute/scalar_compare.go Outdated Show resolved Hide resolved
arrow/compute/scalar_compare.go Outdated Show resolved Hide resolved
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you check CI failures?

@@ -134,4 +136,50 @@ func RegisterScalarComparisons(reg FunctionRegistry) {
reg.AddFunction(ltFn, false)
lteFn := makeFlippedCompare("less_equal", gteFn, EmptyFuncDoc)
reg.AddFunction(lteFn, false)

isOrNotNullKns := kernels.IsNullKernels()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNullKns or isNullOrNotNullKns?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed kernels.IsNullKernels() to kernels.IsNullNotNullKernels() to make it more obvious

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry. I mentioned a local variable name not a function name here.

@zeroshade
Copy link
Member Author

@kou it took a while, but i figured out the cause of the failures. it was the upgrade to the new version of xstrings. v1.4.0 of xstrings.ToCamelCase capitalizes the first letter of the word, while v1.5.0 of ToCamelCase doesn't capitalize the first letter. They really should have marked that as a breaking change :( I think i'll look for something other than xstrings to perform the camel casing there so we don't have to rely on an old version of a package

@zeroshade
Copy link
Member Author

@kou I don't know why the pre-commit is failing though, it passes just fine when I run locally on this branch.

Also for some reason it's not showing the output here so i can know what the actual failure is caused by, which is weird. When i run locally, if there's failures on the linting it outputs it properly to the terminal.

@zeroshade zeroshade requested a review from bkietz September 24, 2024 16:31
@kou
Copy link
Member

kou commented Sep 25, 2024

The lint failure was caused by mixing Go 1.22 and 1.23. See also: golangci/golangci-lint#4908

I've pushed a commit to ensure using Go 1.23 for pre-commit.

@zeroshade zeroshade merged commit 6166f84 into apache:main Sep 25, 2024
22 of 24 checks passed
@zeroshade zeroshade deleted the validity-functions branch September 25, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants