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

Rework package sam/expr/coerce #5086

Merged
merged 3 commits into from
Mar 26, 2024
Merged

Rework package sam/expr/coerce #5086

merged 3 commits into from
Mar 26, 2024

Conversation

mattnibs
Copy link
Collaborator

@mattnibs mattnibs commented Mar 21, 2024

Rework the coerce package so that it uses native values instead of their byte representation.

Fixes #4719

@mattnibs
Copy link
Collaborator Author

mattnibs commented Mar 21, 2024

Shout out to @nwt for doing the heavy lifting on this pr, I merely updated it and helped get it across the finish line.

@mattnibs mattnibs force-pushed the rework-coerce branch 2 times, most recently from db5c4a9 to 42c0663 Compare March 21, 2024 22:28
@mattnibs mattnibs requested a review from a team March 21, 2024 22:29
runtime/sam/expr/eval.go Outdated Show resolved Hide resolved
runtime/sam/expr/eval.go Outdated Show resolved Hide resolved
runtime/sam/expr/eval.go Outdated Show resolved Hide resolved
Rework the coerce package so that it uses native values instead of their
byte representation.
@mattnibs mattnibs requested a review from nwt March 25, 2024 18:44
@philrz
Copy link
Contributor

philrz commented Mar 26, 2024

I just did some test with this branch at commit 5d59954 using the repro steps from #4719 and this confirmed that this branch seems to fully address what's covered in that issue. I've therefore marked that issue as "Fixed" by this PR.

@philrz
Copy link
Contributor

philrz commented Mar 26, 2024

We just discussed at a team meeting that I might add some automated tests based on the #4719 repro, so I'm happy to do that via a push to this branch or a separate PR if this merges first. In prep for that, I did some eyeballing of the output of this exhaustive run through math on all the numeric types:

$ for type1 in $(cat types.txt); do for type2 in $(cat types.txt); do   echo -n "$type1 $type2: " ; echo "{num1: 5($type1), num2: 4($type2)}" | zq -z "yield num1 - num2" -; done; done

On both tip of main (where this issue currently still exists) and this "rework" branch at commit 5d59954, here's a summary of what's changed:

$ diff -y main.out rework.out | grep "\|"
uint32 float16: 1.(float16)					|       uint32 float16: 1.(float32)
uint64 float16: 1.(float16)					|       uint64 float16: 1.
uint64 float32: 1.(float32)					|       uint64 float32: 1.
int32 float16: 1.(float16)					|       int32 float16: 1.(float32)
int64 float16: 1.(float16)					|       int64 float16: 1.
int64 float32: 1.(float32)					|       int64 float32: 1.
float16 uint32: 1.(float16)					|       float16 uint32: 1.(float32)
float16 uint64: 1.(float16)					|       float16 uint64: 1.
float16 int32: 1.(float16)					|       float16 int32: 1.(float32)
float16 int64: 1.(float16)					|       float16 int64: 1.
float16 float32: -Inf(float16)					|       float16 float32: 1.(float32)
float16 float64: -Inf(float16)					|       float16 float64: 1.
float32 uint64: 1.(float32)					|       float32 uint64: 1.
float32 int64: 1.(float32)					|       float32 int64: 1.
float32 float16: -17403.(float32)				|       float32 float16: 1.(float32)
float32 float64: -4616189618054758400.(float32)			|       float32 float64: 1.
float64 float16: -17403.					|       float64 float16: 1.
float64 float32: -1082130427.					|       float64 float32: 1.

Putting aside the fixes to the grossly incorrect math that appear at the bottom (which is all excellent improvement) I also can see that we're using the "bigger" types for math between other mixed types, e.g., the result of math between uint32 and float16 is now putting the result in a float32 instead of a float16 has it had before. This makes sense to me and I've eyeballed what's in the table above and they all similarly make sense to me. So the test I'll propose would effectively lock in the improved behavior as the new normal.

@mattnibs mattnibs merged commit 19b2eb5 into main Mar 26, 2024
3 checks passed
@mattnibs mattnibs deleted the rework-coerce branch March 26, 2024 21:16
nwt added a commit that referenced this pull request Jun 6, 2024
nwt added a commit that referenced this pull request Jun 7, 2024
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.

Smaller float types are broken
3 participants