You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "verma-kartik (via GitHub)" <gi...@apache.org> on 2024/04/09 19:09:42 UTC
[PR] GH-40563: [Go] Unable to JSON marshal float64 arrays which contain a NaN value [arrow]
verma-kartik opened a new pull request, #41109:
URL: https://github.com/apache/arrow/pull/41109
### Rationale for this change
From this [issue](https://github.com/apache/arrow/issues/40563#issuecomment-2043464834).
### What changes are included in this PR?
MarshalJSON() is changed for Float32, Float64 to handle NaN, +Inf and -Inf values. I was only able to handle NaN for Float16.
### Are these changes tested?
These changes are tested. Tests are included in those files where I find it was logically appropriate to do so.
### Are there any user-facing changes?
They will be able to handle corner values for Float32, Float64 now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40563: [Go] Unable to JSON marshal float64 arrays which contain a NaN value [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #41109:
URL: https://github.com/apache/arrow/pull/41109
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40563: [Go] Unable to JSON marshal float64 arrays which contain a NaN value [arrow]
Posted by "verma-kartik (via GitHub)" <gi...@apache.org>.
verma-kartik commented on code in PR #41109:
URL: https://github.com/apache/arrow/pull/41109#discussion_r1559862584
##########
go/arrow/array/float16.go:
##########
@@ -87,10 +87,16 @@ func (a *Float16) GetOneForMarshal(i int) interface{} {
func (a *Float16) MarshalJSON() ([]byte, error) {
vals := make([]interface{}, a.Len())
for i, v := range a.values {
- if a.IsValid(i) {
- vals[i] = v.Float32()
- } else {
+ if !a.IsValid(i) {
vals[i] = nil
+ continue
+ }
+
+ switch {
+ case v.IsNaN():
+ vals[i] = "NaN"
+ default:
+ vals[i] = v.Float32()
}
Review Comment:
Yeah. I had a doubt in this. If I use float16.Inf(), I assume that is "+Inf". What for "-Inf" then?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40563: [Go] Unable to JSON marshal float64 arrays which contain a NaN value [arrow]
Posted by "verma-kartik (via GitHub)" <gi...@apache.org>.
verma-kartik commented on PR #41109:
URL: https://github.com/apache/arrow/pull/41109#issuecomment-2049108204
Oh, this looks wrong. Seems like, I pulled upstream on my branch instead of main.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40563: [Go] Unable to JSON marshal float64 arrays which contain a NaN value [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #41109:
URL: https://github.com/apache/arrow/pull/41109#discussion_r1559788551
##########
go/arrow/array/float16.go:
##########
@@ -87,10 +87,16 @@ func (a *Float16) GetOneForMarshal(i int) interface{} {
func (a *Float16) MarshalJSON() ([]byte, error) {
vals := make([]interface{}, a.Len())
for i, v := range a.values {
- if a.IsValid(i) {
- vals[i] = v.Float32()
- } else {
+ if !a.IsValid(i) {
vals[i] = nil
+ continue
+ }
+
+ switch {
+ case v.IsNaN():
+ vals[i] = "NaN"
+ default:
+ vals[i] = v.Float32()
}
Review Comment:
no check for `Inf`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40563: [Go] Unable to JSON marshal float64 arrays which contain a NaN value [arrow]
Posted by "verma-kartik (via GitHub)" <gi...@apache.org>.
verma-kartik commented on code in PR #41109:
URL: https://github.com/apache/arrow/pull/41109#discussion_r1569212543
##########
go/arrow/array/float16.go:
##########
@@ -87,10 +87,18 @@ func (a *Float16) GetOneForMarshal(i int) interface{} {
func (a *Float16) MarshalJSON() ([]byte, error) {
vals := make([]interface{}, a.Len())
for i, v := range a.values {
- if a.IsValid(i) {
- vals[i] = v.Float32()
- } else {
+ if !a.IsValid(i) {
vals[i] = nil
+ continue
+ }
+
+ switch {
+ case v.IsNaN():
+ vals[i] = "NaN"
+ case v == float16.Inf():
+ vals[i] = "Inf"
+ default:
Review Comment:
I'll do it soon. Been busy with work.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40563: [Go] Unable to JSON marshal float64 arrays which contain a NaN value [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #41109:
URL: https://github.com/apache/arrow/pull/41109#discussion_r1561152657
##########
go/arrow/array/float16.go:
##########
@@ -87,10 +87,16 @@ func (a *Float16) GetOneForMarshal(i int) interface{} {
func (a *Float16) MarshalJSON() ([]byte, error) {
vals := make([]interface{}, a.Len())
for i, v := range a.values {
- if a.IsValid(i) {
- vals[i] = v.Float32()
- } else {
+ if !a.IsValid(i) {
vals[i] = nil
+ continue
+ }
+
+ switch {
+ case v.IsNaN():
+ vals[i] = "NaN"
+ default:
+ vals[i] = v.Float32()
}
Review Comment:
We should add an `IsInf` method to `float16.Num` which can do `return n.bits & 0x7c00`. After that you can simply check the sign bit via `Signbit()` to determine if it's `-Inf` or `+Inf`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40563: [Go] Unable to JSON marshal float64 arrays which contain a NaN value [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #41109:
URL: https://github.com/apache/arrow/pull/41109#issuecomment-2067152517
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 7b62460551f3b8cf2a1c579266268ca75481f329.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/24039485762) has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40563: [Go] Unable to JSON marshal float64 arrays which contain a NaN value [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #41109:
URL: https://github.com/apache/arrow/pull/41109#issuecomment-2048054564
@verma-kartik for the time being, since we are still using the `.tmpl` files and code gen, please update the corresponding template file for `numeric.gen.go.tmpl` rather than only updating the `.go` file.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40563: [Go] Unable to JSON marshal float64 arrays which contain a NaN value [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #41109:
URL: https://github.com/apache/arrow/pull/41109#discussion_r1569124909
##########
go/arrow/array/float16.go:
##########
@@ -87,10 +87,18 @@ func (a *Float16) GetOneForMarshal(i int) interface{} {
func (a *Float16) MarshalJSON() ([]byte, error) {
vals := make([]interface{}, a.Len())
for i, v := range a.values {
- if a.IsValid(i) {
- vals[i] = v.Float32()
- } else {
+ if !a.IsValid(i) {
vals[i] = nil
+ continue
+ }
+
+ switch {
+ case v.IsNaN():
+ vals[i] = "NaN"
+ case v == float16.Inf():
+ vals[i] = "Inf"
+ default:
Review Comment:
Once both positive and negative infinity are handled for float16, I'm fine to merge this
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40563: [Go] Unable to JSON marshal float64 arrays which contain a NaN value [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #41109:
URL: https://github.com/apache/arrow/pull/41109#discussion_r1559793252
##########
go/arrow/array/numeric.gen.go:
##########
@@ -575,10 +588,21 @@ func (a *Float32) GetOneForMarshal(i int) interface{} {
func (a *Float32) MarshalJSON() ([]byte, error) {
vals := make([]interface{}, a.Len())
for i := 0; i < a.Len(); i++ {
- if a.IsValid(i) {
- vals[i] = a.values[i]
- } else {
+ if !a.IsValid(i) {
vals[i] = nil
+ continue
+ }
+
+ f := a.Value(i)
+ switch {
+ case strconv.FormatFloat(float64(f), 'g', -1, 32) == "NaN":
+ vals[i] = "NaN"
+ case strconv.FormatFloat(float64(f), 'g', -1, 32) == "+Inf":
+ vals[i] = "+Inf"
+ case strconv.FormatFloat(float64(f), 'g', -1, 32) == "-Inf":
+ vals[i] = "-Inf"
+ default:
+ vals[i] = f
}
Review Comment:
Please fix the indentation.
Also, let's only call `strconv.FormatFloat` once:
```go
switch v := strconv.FormatFloat(float64(f), 'g', -1, 32) {
case "NaN", "+Inf", "-Inf":
vals[i] = v
default:
vals[i] = f
}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org