Skip to content

fix(flight/flightsql/driver): convert Date32 values - #1039

Merged
zeroshade merged 1 commit into
apache:mainfrom
fallintoplace:fix/flightsql-date32-conversion
Jul 28, 2026
Merged

fix(flight/flightsql/driver): convert Date32 values#1039
zeroshade merged 1 commit into
apache:mainfrom
fallintoplace:fix/flightsql-date32-conversion

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

What changed

Convert FlightSQL Date32 array values to time.Time, matching the existing Date64 behavior.

Why

Date32 previously fell through to ErrNotSupported, preventing valid Date32 columns from being consumed through the database/sql driver.

Tests cover both a concrete Date32 value and a null value.

Validation

go test ./arrow/flight/flightsql/driver

@fallintoplace
fallintoplace requested a review from zeroshade as a code owner July 28, 2026 10:22

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Straightforward gap, and the conversion matches the existing convention exactly. LGTM.

utils.go:105 handled *array.Date64 but had no *array.Date32 case, so Date32 fell through to ErrNotSupported at :118. Confirmed by applying only the test file without the fix:

--- FAIL: Test_fromArrowType/fromArrowType_date32_f22-d32
    err when converting from arrow: type *array.Date32: not supported

The conversion is right and consistent with its sibling: Date32.ToTime() (arrow/datatype_fixedwidth.go:78-80) is time.Unix(0,0).UTC().AddDate(0,0,int(d)), i.e. UTC midnight, structurally identical to Date64.ToTime() at :93-96. Your new line is verbatim the Date64 line, so the two are now consistent — and UTC midnight for a DATE is the standard database/sql convention, so no timezone concern.

No public API or output-semantics change; it strictly converts a previously-erroring case. I also checked there are no ColumnTypeScanType/ColumnTypeDatabaseTypeName hooks in this driver that would need a parallel update, so the fix is self-contained. Test is additive (+16/-0), appended at index 21 with indices 0-20 untouched, and the driver suite passes.

Two notes, neither blocking. TestFromArrowTypeReturnsNilForNullDate32 passes on unpatched main — it exercises the generic arr.IsNull early return at utils.go:62-64 rather than the new code, so it adds no coverage. And the read path still lacks *array.Uint8/16/32/64 even though toArrowDataType accepts those Go types; pre-existing asymmetry, out of scope here but perhaps worth a follow-up.

@zeroshade
zeroshade merged commit 4b93953 into apache:main Jul 28, 2026
23 checks passed
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.

2 participants