zeroshade commented on code in PR #1113:
URL: https://github.com/apache/arrow-go/pull/1113#discussion_r3751930357
##########
arrow/array/record.go:
##########
@@ -415,6 +421,137 @@ func (b *RecordBuilder) NewRecord() arrow.Record {
return b.NewRecordBatch()
}
+type checkpointableBuilder interface {
+ newCheckpoint() checkpointState
+}
+
+type checkpointState interface {
+ capture()
+ restore()
+}
+
+// CheckpointState captures and restores builder state that is not represented
by
+// the builder's length or storage builders. RecordBuilder reuses the same
+// checkpoint for each row, calling Capture before decoding and Restore after a
+// failed decode.
+type CheckpointState interface {
+ // Capture records the current state of the builder.
+ Capture()
+ // Restore returns the builder to the last captured state.
+ Restore()
+}
+
+// CheckpointableBuilder allows custom builders to participate in RecordBuilder
+// row rollback. The returned checkpoint is reused for every row.
+type CheckpointableBuilder interface {
+ // NewCheckpoint returns a reusable checkpoint for the builder.
+ NewCheckpoint() CheckpointState
+}
+
+type checkpointStateAdapter struct {
+ state CheckpointState
+}
+
+func (s *checkpointStateAdapter) capture() { s.state.Capture() }
+func (s *checkpointStateAdapter) restore() { s.state.Restore() }
+
+type storageBuilder interface {
+ StorageBuilder() Builder
+}
+
+type builderCheckpoint struct {
+ builder Builder
+ length int
+ children []*builderCheckpoint
+ state checkpointState
+ lastUnmarshalled interface{}
+ unmarshalled bool
+ lastStr *string
+}
+
+func newBuilderCheckpoint(builder Builder) *builderCheckpoint {
+ checkpoint := &builderCheckpoint{
+ builder: builder,
+ }
+ if checkpointable, ok := builder.(checkpointableBuilder); ok {
+ checkpoint.state = checkpointable.newCheckpoint()
+ } else if checkpointable, ok := builder.(CheckpointableBuilder); ok {
+ checkpoint.state = &checkpointStateAdapter{state:
checkpointable.NewCheckpoint()}
+ }
+
+ switch builder := builder.(type) {
Review Comment:
Minor: any future builder type with children that isn't added to this switch
will silently skip rollback — the same failure mode this PR fixes, with no
compile-time guard. Worth a short comment here noting that new nested builders
must be registered, or a test that reflects over the known builder types.
##########
arrow/extensions/timestamp_with_offset.go:
##########
@@ -439,6 +439,31 @@ func (b *TimestampWithOffsetBuilder) NewArray()
arrow.Array {
return b.NewExtensionArray()
}
+// Resize adjusts the capacity of the builder and resets the current run-end
+// offset state.
+func (b *TimestampWithOffsetBuilder) Resize(n int) {
+ b.ExtensionBuilder.Resize(n)
+ b.lastOffset = noLastOffset
Review Comment:
Blocking: this resets the run-end tracker on every `Resize`, including
capacity growth.
`builder.reserve()` calls `resize()` whenever `length+elements > capacity`,
so this fires during ordinary appending, not just on explicit user calls. When
it fires mid-run, `lastOffset` goes back to `noLastOffset`, the next append
takes the `offsetMinutes != b.lastOffset` branch, and `offsets.Append(1)`
starts a **new run holding the same value as the previous run**.
That's the condition the existing comment in `AppendValues` explicitly
preserves against: *"lastOffset is only updated when a new run starts, so a
null row never splits a contiguous run into two adjacent runs sharing the same
value."*
Effects: runs fragment (losing the compression REE exists for), and run
boundaries become a function of allocation growth points rather than of the
data — so the same input encodes differently depending on `Reserve` history.
Unlikely to surface in small unit tests.
Suggested fix: drop this override. `timestampWithOffsetCheckpoint.Restore()`
already restores `lastOffset` on the rollback path, which is the only path that
needs it.
--
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]
