Skip to content

fix: data race on Arrow v12 shared timestamp types (prewarm GetToTimeFunc) - #465

Open
TangoEnSkai wants to merge 1 commit into
databricks:mainfrom
TangoEnSkai:fix/arrow-v12-timestamp-race
Open

fix: data race on Arrow v12 shared timestamp types (prewarm GetToTimeFunc)#465
TangoEnSkai wants to merge 1 commit into
databricks:mainfrom
TangoEnSkai:fix/arrow-v12-timestamp-race

Conversation

@TangoEnSkai

Copy link
Copy Markdown

Context

close #179

Arrow v12's TimestampType.GetToTimeFunc lazily caches the type's *time.Location (via GetZone, which writes t.loc with no synchronization — apache/arrow#38795, fixed only in Arrow versions this driver can't move to yet, see #228). NewArrowRowScanner calls GetToTimeFunc on the shared arrow.FixedWidthTypes.Timestamp_us singleton for every result set, so the first concurrent queries race on that call. In the issue thread, @7phs posted the init() prewarm workaround and @bombsimon asked for it to live in the driver instead of every consumer — this PR does exactly that.

What

  • Add a package init() in internal/rows/arrowbased that calls GetToTimeFunc() once on all four shared fixed-width timestamp singletons (Timestamp_s/_ms/_us/_ns), so the location cache is written before any concurrency is possible and every later call is a plain read.
  • Add TestSharedTimestampGetToTimeFuncConcurrency, which hits the shared singletons from 32 goroutines. Verified both ways locally: with the init() it passes under -race; with the init() renamed away it fails with race detected during execution of test.
  • CHANGELOG entry under ## Unreleased.

Why

  • Prewarming is the minimal fix that works while the driver stays on Arrow v12 (the v12 pin is deliberate for backward compatibility, per Revert update Apache Arrow to v16 #228). Once warmed, GetZone only ever reads t.loc, so the race is gone rather than hidden.
  • Doing it in the driver removes the footgun @bombsimon described: consumers currently have to import Arrow — a transitive dependency — and replicate the init() in their own code just to use databricks-sql-go safely from multiple goroutines.
  • All four singletons are warmed (not just Timestamp_us, the one NewArrowRowScanner uses) to match the workaround users already deploy and to keep any other shared-singleton use safe.

Completion Criteria

  • go build ./..., go vet ./..., gofmt clean on touched files
  • Full unit suite passes (go test ./...)
  • New test passes under -race with the fix, fails under -race without it
  • CI green

Note for reviewers: go test -race ./internal/rows/arrowbased/ also reports pre-existing, unrelated races in TestCloudFetchIterator_CloseReleasesInFlightDownloads / TestCloudFetchIterator_CloseReleasesAfterRetry (a container/list race in the CloudFetch iterator). Those fail identically on main without this change.

…Func)

Arrow v12's TimestampType.GetToTimeFunc lazily caches the type's
*time.Location via GetZone without synchronization (apache/arrow#38795,
fixed only in later Arrow versions the driver cannot move to yet, see
issue databricks#228). NewArrowRowScanner calls GetToTimeFunc on the shared
arrow.FixedWidthTypes.Timestamp_us singleton for every result set, so
concurrent queries race on that first call, as reported in issue databricks#179.

Warm the cache for all four shared fixed-width timestamp singletons in a
package init, before any concurrency is possible, so every later call is
a plain read. This is the same workaround users currently have to apply
in their own code; doing it in the driver removes the need to import
Arrow (a transitive dependency) just to use databricks-sql-go safely
from multiple goroutines.

The new regression test fails under the race detector if the init is
removed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: TangoEnSkai <21152231+TangoEnSkai@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Data race of reading row value of timestamp data type

1 participant