zeroshade opened a new pull request, #823:
URL: https://github.com/apache/arrow-go/pull/823
### Rationale for this change
`go vet ./arrow/array/` reports:
```
arrow/array/numeric_generic.go:39:9: return copies lock value:
numericArray[T] contains array contains sync/atomic.Int64 contains
sync/atomic.noCopy
```
The `newNumericData` helper instantiated a `numericArray[T]` zero value,
mutated it, and returned it **by value**. Returning this value copies the
embedded `array` struct, which contains an `atomic.Int64` refcount with a
`noCopy` marker. The 15 `New<T>Data` callers then copy the value a second time
when embedding it into their wrapper struct literal (e.g. `return
&Int64{numericArray: newNumericData[int64](data)}`).
In practice the refcount value is only copied during single-threaded
construction before the resulting `*Type` pointer is shared, so no race exists
today (this is acknowledged in the original commit that introduced
`atomic.Int64` — #326). However, the warning is permanent noise in `go vet`
output, masks any future *real* copylocks regressions in this code path, and
signals a fragile pattern that one careless refactor would turn into a real bug.
### What changes are included in this PR?
- Replace `newNumericData[T](data) numericArray[T]` with
`initNumericArray[T](a *numericArray[T], data)`. The helper now initializes an
already-allocated `numericArray[T]` in place — no value return, no copy.
- Update all 15 `New<T>Data` constructors (`NewInt64Data`, `NewUint64Data`,
`NewInt32Data`, `NewUint32Data`, `NewInt16Data`, `NewUint16Data`,
`NewInt8Data`, `NewUint8Data`, `NewFloat32Data`, `NewFloat64Data`,
`NewDate32Data`, `NewDate64Data`, `NewTime32Data`, `NewTime64Data`,
`NewDurationData`) to allocate the concrete wrapper on the heap first, then
pass a pointer to the embedded `numericArray` field via Go field promotion.
This mirrors the existing idiom already used by `NewStringData`,
`NewBinaryData`, and `newDecimalData` in this same package.
### Are these changes tested?
- `go vet ./arrow/array/` is clean for `numeric_generic.go` (an unrelated
`extension.go:100` warning involving `reflect.ValueOf` remains and is out of
scope for this issue).
- `go test ./arrow/...` passes (15 packages).
- `go test ./parquet/pqarrow/...` passes (excluding pre-existing tests that
require `PARQUET_TEST_DATA`).
### Are there any user-facing changes?
No. `numericArray[T]` is unexported and the public `New<T>Data` signatures
and runtime behavior are unchanged. The number of heap allocations per
constructor is the same as before — fields are now constructed directly inside
the heap-allocated wrapper rather than copied into it.
Closes #821
--
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]
