You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "matan129 (via GitHub)" <gi...@apache.org> on 2026/06/10 13:02:56 UTC
[PR] fix(avro): append raw bytes [arrow-go]
matan129 opened a new pull request, #850:
URL: https://github.com/apache/arrow-go/pull/850
### Rationale for this change
`appendBinaryData` has no case for `[]byte`, so a plain bytes value falls into the`fmt.Append` fallback, which results in a string-formated value.
```go
schema := `{"type":"record","name":"r","fields":[{"name":"f","type":"bytes"}]}`
var buf bytes.Buffer
enc, _ := ocf.NewEncoder(schema, &buf)
enc.Encode(map[string]any{"f": []byte{0x00, 0x01, 0xfe, 0xff}})
enc.Close()
r, _ := avro.NewOCFReader(&buf, avro.WithChunk(-1))
r.Next()
fmt.Printf("%x\n", r.RecordBatch().Column(0).(*array.Binary).Value(0))
// got: 5b30203120323534203235355d — the 13-byte text "[0 1 254 255]"
// want: 0001feff
```
### What changes are included in this PR?
- `appendBinaryData`: append `[]bytes` as is & append `string` as its raw bytes
- `appendStringData`: handle `[]byte`.
- `testdata.ByteArray.MarshalJSON`: Remove `fmt.Sprint(<bytes>)` to match the new behaviour
### Are these changes tested?
Yes. Added regression test: `TestOCFReaderBytesValues` covering plain bytes and `["null","bytes"]`
### Are there any user-facing changes?
Yes. Avro bytes columns previously decoded to corrupted text now decode to the actual payload.
--
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] fix(avro): append raw bytes [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #850:
URL: https://github.com/apache/arrow-go/pull/850#discussion_r3390422692
##########
arrow/avro/reader_types.go:
##########
@@ -594,6 +594,10 @@ func appendBinaryData(b *array.BinaryBuilder, data interface{}) {
switch dt := data.(type) {
case nil:
b.AppendNull()
+ case []byte:
+ b.Append(dt)
Review Comment:
These explicit `[]byte`/`string` cases are the right fix. Forward-looking nit: the `default:` branch a few lines below in this function — `b.Append(fmt.Append([]byte{}, data))` — is the exact silent fallback that produced the corrupted text this PR fixes. Now that `nil`/`[]byte`/`string`/`map[string]any` are all handled it's unreachable for hamba's real outputs, so this is a good opportunity to turn it (and the identical `b.Append(fmt.Sprint(data))` default in `appendStringData`) into a hard error/panic on the unexpected `%T`. That way the next decoder/type mismatch fails loudly instead of silently corrupting a column. Non-blocking.
##########
arrow/avro/reader_types.go:
##########
@@ -594,6 +594,10 @@ func appendBinaryData(b *array.BinaryBuilder, data interface{}) {
switch dt := data.(type) {
case nil:
b.AppendNull()
+ case []byte:
+ b.Append(dt)
+ case string:
+ b.Append([]byte(dt))
case map[string]any:
switch ct := dt["bytes"].(type) {
Review Comment:
Pre-existing (not introduced by this PR) but right next to the change: the `default` of this `dt["bytes"]` switch does `b.Append(ct.([]byte))`, an unchecked type assertion that will panic if the union's `bytes` value isn't a `[]byte`. `appendFixedSizeBinaryData` handles the same shape more defensively with a typed `case []byte: b.Append(v)` and lets anything else fall through. Might be worth mirroring that here while you're in the file. Optional.
##########
arrow/avro/reader_types.go:
##########
@@ -594,6 +594,10 @@ func appendBinaryData(b *array.BinaryBuilder, data interface{}) {
switch dt := data.(type) {
case nil:
b.AppendNull()
+ case []byte:
+ b.Append(dt)
+ case string:
+ b.Append([]byte(dt))
Review Comment:
Optional: per hamba's decoder a `bytes` field (and a nullable `["null","bytes"]` union) decodes into `any` as a bare `[]byte`, never a Go `string`, so this `case string` is effectively dead on the normal decode path. It's harmless as defensive code, but note the `appendFixedSizeBinaryData` you're mirroring has no `string` case — for consistency you could drop it. Your call.
--
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] fix(avro): append raw bytes [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #850:
URL: https://github.com/apache/arrow-go/pull/850
--
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] fix(avro): append raw bytes [arrow-go]
Posted by "matan129 (via GitHub)" <gi...@apache.org>.
matan129 commented on code in PR #850:
URL: https://github.com/apache/arrow-go/pull/850#discussion_r3390568134
##########
arrow/avro/reader_types.go:
##########
@@ -594,6 +594,10 @@ func appendBinaryData(b *array.BinaryBuilder, data interface{}) {
switch dt := data.(type) {
case nil:
b.AppendNull()
+ case []byte:
+ b.Append(dt)
Review Comment:
Done in 6a7b49b. Went with an error return rather than a panic since the plumbing already exists: `appendFunc` returns `error` and `loadDatum` propagates it to `OCFReader.Err()`, same as the decimal appenders. Both `appendBinaryData` and `appendStringData` now error on any type outside what hamba produces, with a test covering the error paths.
##########
arrow/avro/reader_types.go:
##########
@@ -594,6 +594,10 @@ func appendBinaryData(b *array.BinaryBuilder, data interface{}) {
switch dt := data.(type) {
case nil:
b.AppendNull()
+ case []byte:
+ b.Append(dt)
+ case string:
+ b.Append([]byte(dt))
Review Comment:
Dropped it in 6a7b49b. With the default now returning an error, a string reaching a binary column fails loudly instead of being silently coerced, which seems preferable to keeping the dead case.
##########
arrow/avro/reader_types.go:
##########
@@ -594,6 +594,10 @@ func appendBinaryData(b *array.BinaryBuilder, data interface{}) {
switch dt := data.(type) {
case nil:
b.AppendNull()
+ case []byte:
+ b.Append(dt)
+ case string:
+ b.Append([]byte(dt))
case map[string]any:
switch ct := dt["bytes"].(type) {
Review Comment:
Done in 6a7b49b — the union branch now uses a typed `case []byte` like `appendFixedSizeBinaryData`, and anything else falls into the new error default rather than panicking.
--
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] fix(avro): append raw bytes [arrow-go]
Posted by "matan129 (via GitHub)" <gi...@apache.org>.
matan129 commented on PR #850:
URL: https://github.com/apache/arrow-go/pull/850#issuecomment-4673730212
Thanks for the second pass.
* Note 1 is done in `b76328e` the `dt["string"]` union switch now errors on unexpected types, matching `appendBinaryData`, with test coverage
* For note 2, I went ahead and fixed it in this PR. Now `loadDatum` propagate errors on all paths.
--
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