serramatutu commented on code in PR #833:
URL: https://github.com/apache/arrow-go/pull/833#discussion_r3864335360
##########
arrow/array/util.go:
##########
@@ -296,7 +296,11 @@ func RecordToJSON(rec arrow.RecordBatch, w io.Writer)
error {
cols := make(map[string]interface{})
for i := 0; int64(i) < rec.NumRows(); i++ {
for j, c := range rec.Columns() {
- cols[fields[j].Name] = c.GetOneForMarshal(i)
+ if rec.Schema().Field(j).Nullable && c.IsNull(i) {
Review Comment:
Per this: https://github.com/serramatutu/arrow-internal-nulls
In C++, it looks like the general approach is "validate all data in", then
assume it's OK/consistent when "writing out". I.e it is an invariant that the
data is correct when writing out.
If we're very pedantic about this, we should just revert this change and use
`c.GetOneForMarshal(i)` as we should assume `IsNull(i)` will always return
false. This is what C++ does actually, so here we're doing even more validation
by checking `rec.Schema().Field(j).Nullable`.
Sources:
- `JsonWriter::WriteArray` uses an `ArrayWriter`:
https://github.com/apache/arrow/blob/b12755a1cc41ee5e9b1874ece3bcc92d6d3e3e78/cpp/src/arrow/integration/json_internal.cc#L2079
- `ArrayWriter::WriteDataValues` does not check for the schema:
https://github.com/apache/arrow/blob/b12755a1cc41ee5e9b1874ece3bcc92d6d3e3e78/cpp/src/arrow/integration/json_internal.cc#L505
If the user has messed around with the data:
- by tweaking memory directly: they need to ensure their code respects
invariants OR call ValidateFull
- by using a public API: the API will always leave the array in a consistent
internal state
This is my rationale for calling GetOneForMarshal() here: we assume the data
is correct, if it's not then it's UB.
--
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]
