You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2026/06/11 19:48:30 UTC
[PR] fix(parquet): add WriteBatchSpacedWithError to surface spaced-write failures [arrow-go]
zeroshade opened a new pull request, #852:
URL: https://github.com/apache/arrow-go/pull/852
### Rationale for this change
Closes #826.
Across all 8 generated `*ColumnChunkWriter` types in `parquet/file/column_writer_types.gen.go`, `WriteBatch` and `WriteBatchSpaced` are an asymmetric pair:
- `WriteBatch` returns `(valueOffset int64, err error)` and opens with a `defer recover()` that converts any panic into a returned `error` via `utils.FormatRecoveredError`.
- `WriteBatchSpaced` returns nothing, has no `defer recover()`, and on the `doBatches` (non-byte-array) path silently discards the `error` returned by `commitWriteAndCheckPageLimit`.
So a write failure during a spaced write is either **silently discarded** (numeric/boolean/fixed paths) or **escapes as a panic out of the calling goroutine** (the byte-array branch's `panic(err)`, plus other internal panics such as dictionary-fallback bookkeeping) — because, unlike `WriteBatch`, there is no `recover()` to turn it into an `error`.
This is the same class of "panic escapes / error lost on the public API" bug as #820 (fixed in #824), on a different code path. It is reachable through `pqarrow` — `(*pqarrow.FileWriter)` drives `WriteBatchSpaced` for **nullable** columns — so for consumers writing nullable columns to a network-attached sink (GCS, S3 multipart, HDFS, etc.), a transient downstream failure on the spaced path crashes the worker process or silently drops the error instead of returning it.
### What changes are included in this PR?
1. **New method `WriteBatchSpacedWithError(values, defLevels, repLevels, validBits, validBitsOffset) (int64, error)`** generated for all 8 column-writer types from `column_writer_types.gen.go.tmpl`. It mirrors `WriteBatch`:
- opens with `defer func() { if r := recover(); r != nil { err = utils.FormatRecoveredError(...) } }()`, and
- checks the `error` from `commitWriteAndCheckPageLimit` on **every** branch (the `doBatches` branch previously ignored it), `panic`-ing so the deferred recover surfaces it as a returned `error`.
2. **`WriteBatchSpaced` is left byte-for-byte unchanged** — same signature, same behavior. This keeps the change **non-breaking and purely additive**, exactly the approach #824 took with `NewParquetWriter` / `NewParquetWriterWithError`. Existing callers of `WriteBatchSpaced` are unaffected; callers that want error handling opt into `WriteBatchSpacedWithError`.
3. **`pqarrow`'s dense-array encoder (`writeDenseArrow`) now calls `WriteBatchSpacedWithError`** at all 12 spaced-write sites and propagates the error through its existing `(err error)` return, so nullable-column writes report sink failures as errors instead of crashing the goroutine or losing the error.
The diff to the generated file and the template is additions-only (no existing generated method bodies change).
### Are these changes tested?
Yes. New `TestWriteBatchSpacedWithErrorPropagatesWriteFailure` in `parquet/file/column_writer_test.go` uses the existing `mockpagewriter` to fail `WriteDataPage`, with a tiny `DataPageSize` and dictionary disabled to force a flush mid-call. It asserts:
- `WriteBatchSpacedWithError` returns the wrapped failure (`errors.Is(err, failureErr)`), rather than panicking or discarding it; and
- the legacy `WriteBatchSpaced` does **not** panic out of the call (its original behavior is preserved).
The full `parquet/file/...` and `parquet/pqarrow/...` suites pass (`PARQUET_TEST_DATA` pointed at `parquet-testing/data`), and `go vet` is clean.
### Are there any user-facing changes?
**No breaking changes.**
- `WriteBatchSpaced(...)` — signature and behavior unchanged.
- `WriteBatchSpacedWithError(...) (int64, error)` — new exported method on each generated column-writer type. Adding exported methods is non-breaking in Go.
- `pqarrow` public signatures are unchanged; the dense-array nullable write path now returns transient sink failures as an `error` (callers already checking `err` get strictly better behavior).
### Out of scope (separate follow-up)
`WriteBitmapBatchSpaced` (Boolean) has the same void-signature / no-recover shape; it is not on the `WriteBatchSpaced` path this issue targets and can be addressed separately if desired.
--
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(parquet): add WriteBatchSpacedWithError to surface spaced-write failures [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #852:
URL: https://github.com/apache/arrow-go/pull/852#discussion_r3439076976
##########
parquet/file/column_writer_types.gen.go.tmpl:
##########
@@ -269,6 +269,97 @@ func (w *{{.Name}}ColumnChunkWriter) WriteBatchSpaced(values []{{.name}}, defLev
{{- end}}
}
+// WriteBatchSpacedWithError behaves like WriteBatchSpaced but reports a write
+// failure as an error instead of panicking or silently discarding it,
+// mirroring the error handling of WriteBatch. Prefer this method on write
+// paths whose sink may fail (for example a network-attached writer).
+func (w *{{.Name}}ColumnChunkWriter) WriteBatchSpacedWithError(values []{{.name}}, defLevels, repLevels []int16, validBits []byte, validBitsOffset int64) (valueOffset int64, err error) {
Review Comment:
Done in 975d0524.
1. **Deprecated** — both `WriteBatchSpaced` (all 8 column-writer types) and `WriteBitmapBatchSpaced` now carry a `// Deprecated:` notice pointing at their `…WithError` counterparts.
2. **No more duplication** — each now just defers to the `…WithError` variant and panics if it returns an error, so the duplicated bodies are gone (−265 lines in the generated file; template + regen).
One behavior note worth your eyes: the legacy methods now fail loudly on a write error instead of silently discarding it on the numeric/boolean/fixed paths (the byte-array path already panicked). Because the delegation routes through `…WithError`'s `recover`, the panic payload is now the `FormatRecoveredError("unknown error type", …)` wrapper rather than the raw error — `errors.Is(…, original)` still resolves through the `%w` chain, but the exact value differs for the byte-array/FLBA paths that previously panicked with the raw error. Happy to re-panic the unwrapped cause for those two if you'd rather preserve their exact original payload.
Internal callers are off the deprecated API: `pqarrow` already uses `WriteBatchSpacedWithError`, and the spaced-write test helper in `internal/testutils` was migrated too. The spaced-write failure tests now assert the deprecated methods panic with the underlying error.
--
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(parquet): add WriteBatchSpacedWithError to surface spaced-write failures [arrow-go]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #852:
URL: https://github.com/apache/arrow-go/pull/852#discussion_r3410340185
##########
parquet/file/column_writer_types.gen.go.tmpl:
##########
@@ -269,6 +269,97 @@ func (w *{{.Name}}ColumnChunkWriter) WriteBatchSpaced(values []{{.name}}, defLev
{{- end}}
}
+// WriteBatchSpacedWithError behaves like WriteBatchSpaced but reports a write
+// failure as an error instead of panicking or silently discarding it,
+// mirroring the error handling of WriteBatch. Prefer this method on write
+// paths whose sink may fail (for example a network-attached writer).
+func (w *{{.Name}}ColumnChunkWriter) WriteBatchSpacedWithError(values []{{.name}}, defLevels, repLevels []int16, validBits []byte, validBitsOffset int64) (valueOffset int64, err error) {
Review Comment:
(1) Could we deprecate WriteBatchSpaced?
(2) Could WriteBatchSpaced be written in terms of WriteBatchSpacedWithError instead of duplicating the code entirely? (e.g. defer to WriteBatchSpacedWithError and panic if it returns an error)
--
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(parquet): add WriteBatchSpacedWithError to surface spaced-write failures [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #852:
URL: https://github.com/apache/arrow-go/pull/852
--
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(parquet): add WriteBatchSpacedWithError to surface spaced-write failures [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #852:
URL: https://github.com/apache/arrow-go/pull/852#discussion_r3439174101
##########
parquet/file/column_writer_types.gen.go.tmpl:
##########
@@ -269,6 +269,97 @@ func (w *{{.Name}}ColumnChunkWriter) WriteBatchSpaced(values []{{.name}}, defLev
{{- end}}
}
+// WriteBatchSpacedWithError behaves like WriteBatchSpaced but reports a write
+// failure as an error instead of panicking or silently discarding it,
+// mirroring the error handling of WriteBatch. Prefer this method on write
+// paths whose sink may fail (for example a network-attached writer).
+func (w *{{.Name}}ColumnChunkWriter) WriteBatchSpacedWithError(values []{{.name}}, defLevels, repLevels []int16, validBits []byte, validBitsOffset int64) (valueOffset int64, err error) {
Review Comment:
Pushed `f4b92fc5` to address the panic-payload nuance I mentioned above.
The deprecated `WriteBatchSpaced` / `WriteBitmapBatchSpaced` now re-panic the **unwrapped** cause (via a small shared `panicCause` helper) instead of `panic(err)`, so the byte-array / FLBA paths surface the original error value again rather than the `FormatRecoveredError("unknown error type", …)` wrapper they'd otherwise inherit from delegating through `…WithError`. `errors.Is(…, original)` still resolves, and non-error panics fall back to the wrapper.
Net result: those two paths keep their exact pre-deprecation panic payload, while the numeric/boolean paths still fail loud instead of silently dropping the error. `go build`/`vet`/`staticcheck` are clean and the `parquet/file` + `parquet/pqarrow` suites pass.
--
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(parquet): add WriteBatchSpacedWithError to surface spaced-write failures [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #852:
URL: https://github.com/apache/arrow-go/pull/852#issuecomment-4693037598
Both are technically part of the public API and I'm trying to avoid making breaking changes, it isn't inside of an `internal` package and it starts with a capital letter, so it's publicly accessible.
--
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