diff --git a/driver_e2e_test.go b/driver_e2e_test.go index 4bdaeb84..7a057c94 100644 --- a/driver_e2e_test.go +++ b/driver_e2e_test.go @@ -709,3 +709,42 @@ func TestE2ECloudFetchExactRowCount(t *testing.T) { require.Equal(t, int64(wantRows), rowCount, "CloudFetch must surface exactly the requested rows, with no Arrow padding") } + +// TestE2ETimeTypeScan is the reproducer for databricks/databricks-sql-go#505: +// a SQL TIME column arrives over the Thrift Arrow path as a time64[ns] array, +// which the arrow row scanner had no arm for, so the whole result failed with +// "arrow row scanner unhandled type time64[ns]" before any row was returned. +// A TIME must scan as a wall-clock time-of-day (no date, no location) carrying +// the correct hour/minute/second/fractional component. +func TestE2ETimeTypeScan(t *testing.T) { + host, httpPath, token := pecoTestingCreds(t) + + connector, err := NewConnector( + WithServerHostname(host), + WithPort(443), + WithHTTPPath(httpPath), + WithAccessToken(token), + ) + require.NoError(t, err) + + db := sql.OpenDB(connector) + defer db.Close() //nolint:errcheck + + ctx := context.Background() + + rows, err := db.QueryContext(ctx, "SELECT TIME '09:15:00.123456'") + require.NoError(t, err) + defer rows.Close() //nolint:errcheck + + require.True(t, rows.Next(), "expected one row") + + var dest time.Time + require.NoError(t, rows.Scan(&dest), "TIME column must scan without the unhandled-type error") + + assert.Equal(t, 9, dest.Hour()) + assert.Equal(t, 15, dest.Minute()) + assert.Equal(t, 0, dest.Second()) + assert.Equal(t, 123456000, dest.Nanosecond()) + + require.NoError(t, rows.Err()) +} diff --git a/internal/arrowscan/arrowscan.go b/internal/arrowscan/arrowscan.go index c6705955..5333be2b 100644 --- a/internal/arrowscan/arrowscan.go +++ b/internal/arrowscan/arrowscan.go @@ -214,6 +214,17 @@ func scanCell(col arrow.Array, row int, loc *time.Location, keys *StructKeyCache // round-trips. The Thrift default path never hits this — it receives TIMESTAMP // as a preformatted server string — so this divergence is kernel-only. return inLocation(timestampToTime(int64(c.Value(row)), dt.Unit), loc), nil + case *array.Time32: + // SQL TIME (time-of-day, no calendar date, no timezone). Match the Thrift + // path's timeValueContainer: render as a wall-clock time.Time anchored on + // the Unix epoch in UTC and do NOT apply loc — a zoneless TIME must not be + // shifted by cfg.Location (that would move the hour and could roll the + // anchor date off the epoch). + dt := col.DataType().(*arrow.Time32Type) + return timeOfDayToTime(int64(c.Value(row)), dt.Unit), nil + case *array.Time64: + dt := col.DataType().(*arrow.Time64Type) + return timeOfDayToTime(int64(c.Value(row)), dt.Unit), nil case *array.Decimal128: dt := col.DataType().(*arrow.Decimal128Type) if options.DecimalAsFloat { @@ -380,6 +391,36 @@ func timestampToTime(v int64, unit arrow.TimeUnit) time.Time { } } +// timeOfDayToTime renders an arrow time32/time64 value (units since midnight) as +// a wall-clock time.Time anchored on the Unix epoch date in UTC, matching the +// Thrift path's timeValueContainer. The value is zoneless, so it is intentionally +// left in UTC (no loc applied). The multiply cannot overflow: a valid TIME is +// bounded by 24h, so even in nanoseconds units*nsPerUnit stays well within int64. +func timeOfDayToTime(unitsSinceMidnight int64, unit arrow.TimeUnit) time.Time { + var nsPerUnit int64 + switch unit { + case arrow.Second: + nsPerUnit = int64(time.Second) + case arrow.Millisecond: + nsPerUnit = int64(time.Millisecond) + case arrow.Microsecond: + nsPerUnit = int64(time.Microsecond) + default: // arrow.Nanosecond + nsPerUnit = 1 + } + return time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC). + Add(time.Duration(unitsSinceMidnight * nsPerUnit)) +} + +// formatTimeOfDay renders an arrow time32/time64 value as the zoneless +// time-of-day string the Thrift path emits for a nested TIME +// (timeValueContainer.ValueString: "15:04:05.999999999"). Top-level TIME returns +// a time.Time (see the scanCell Time32/Time64 arms); nested TIME must instead be +// a bare string so the JSON leaf does not leak the epoch anchor date. +func formatTimeOfDay(unitsSinceMidnight int64, unit arrow.TimeUnit) string { + return timeOfDayToTime(unitsSinceMidnight, unit).Format("15:04:05.999999999") +} + // inLocation renders t in loc, matching the Thrift path's .In(location); a nil // loc leaves the value in UTC (arrow's ToTime default). func inLocation(t time.Time, loc *time.Location) time.Time { @@ -444,6 +485,14 @@ func writeJSON(b *strings.Builder, col arrow.Array, row int, loc *time.Location, // "3.140000104904175". The Thrift nested path marshals the native float32, // so widening here would break byte-parity for ARRAY/MAP/STRUCT<…FLOAT…>. return writeScalarJSON(b, c.Value(row)) + case *array.Time32: + // Nested SQL TIME: emit the zoneless time-of-day string, matching the + // Thrift path's timeValueContainer.ValueString (marshalScalar json.Marshals + // it). Falling through to writeScalarJSON would quote t.String() and leak + // the 1970-01-01 epoch anchor date, diverging from Thrift. + return writeScalarJSON(b, formatTimeOfDay(int64(c.Value(row)), col.DataType().(*arrow.Time32Type).Unit)) + case *array.Time64: + return writeScalarJSON(b, formatTimeOfDay(int64(c.Value(row)), col.DataType().(*arrow.Time64Type).Unit)) default: v, err := scalarForJSON(col, row, loc) if err != nil { diff --git a/internal/arrowscan/arrowscan_test.go b/internal/arrowscan/arrowscan_test.go index e68745eb..a60adfb2 100644 --- a/internal/arrowscan/arrowscan_test.go +++ b/internal/arrowscan/arrowscan_test.go @@ -104,11 +104,12 @@ func TestScanCellScalars(t *testing.T) { t.Run("unsupported_type_errors", func(t *testing.T) { // An unhandled arrow type must error, not return a silently wrong value. - // Duration/MonthInterval are now handled (see TestScanCellInterval); use a - // type with no scan arm. - b := array.NewTime32Builder(pool, &arrow.Time32Type{Unit: arrow.Second}) + // Duration/MonthInterval are now handled (see TestScanCellInterval) and + // Time32/Time64 are now handled (see TestScanCellTimeOfDay); use a type with + // no scan arm. + b := array.NewMonthDayNanoIntervalBuilder(pool) defer b.Release() - b.Append(1000) + b.Append(arrow.MonthDayNanoInterval{Months: 1, Days: 2, Nanoseconds: 3}) arr := b.NewArray() defer arr.Release() if _, err := ScanCell(arr, 0, nil); err == nil { @@ -225,6 +226,83 @@ func TestScanCellInterval(t *testing.T) { } } +// SQL TIME (arrow time32/time64) arrives as units-since-midnight and must render +// to the same epoch-anchored UTC wall-clock time.Time the Thrift path produces +// (timeValueContainer). It is zoneless, so loc must NOT shift the value. +func TestScanCellTimeOfDay(t *testing.T) { + pool := memory.NewGoAllocator() + // 13:01:01.5 as the wall-clock time-of-day both paths must return. + want := time.Date(1970, time.January, 1, 13, 1, 1, 500_000_000, time.UTC) + + t.Run("time32_seconds", func(t *testing.T) { + b := array.NewTime32Builder(pool, &arrow.Time32Type{Unit: arrow.Second}) + defer b.Release() + b.Append(arrow.Time32(13*3600 + 1*60 + 1)) // 13:01:01 + arr := b.NewArray() + defer arr.Release() + v, err := ScanCell(arr, 0, nil) + if err != nil { + t.Fatal(err) + } + got := v.(time.Time) + wantSec := time.Date(1970, time.January, 1, 13, 1, 1, 0, time.UTC) + if !got.Equal(wantSec) { + t.Errorf("got %v, want %v", got, wantSec) + } + }) + + t.Run("time32_millis", func(t *testing.T) { + b := array.NewTime32Builder(pool, &arrow.Time32Type{Unit: arrow.Millisecond}) + defer b.Release() + b.Append(arrow.Time32((13*3600+1*60+1)*1000 + 500)) // 13:01:01.500 + arr := b.NewArray() + defer arr.Release() + v, err := ScanCell(arr, 0, nil) + if err != nil { + t.Fatal(err) + } + if got := v.(time.Time); !got.Equal(want) { + t.Errorf("got %v, want %v", got, want) + } + }) + + t.Run("time64_micros", func(t *testing.T) { + b := array.NewTime64Builder(pool, &arrow.Time64Type{Unit: arrow.Microsecond}) + defer b.Release() + b.Append(arrow.Time64((13*3600+1*60+1)*1_000_000 + 500_000)) // 13:01:01.500000 + arr := b.NewArray() + defer arr.Release() + v, err := ScanCell(arr, 0, nil) + if err != nil { + t.Fatal(err) + } + if got := v.(time.Time); !got.Equal(want) { + t.Errorf("got %v, want %v", got, want) + } + }) + + t.Run("time64_nanos_zoneless_ignores_loc", func(t *testing.T) { + loc, err := time.LoadLocation("America/New_York") + if err != nil { + t.Skipf("tz database unavailable: %v", err) + } + b := array.NewTime64Builder(pool, &arrow.Time64Type{Unit: arrow.Nanosecond}) + defer b.Release() + b.Append(arrow.Time64((13*3600+1*60+1)*int64(time.Second) + 500_000_000)) + arr := b.NewArray() + defer arr.Release() + // A zoneless TIME must not be shifted by loc: same wall-clock, still UTC. + v, err := ScanCell(arr, 0, loc) + if err != nil { + t.Fatal(err) + } + got := v.(time.Time) + if !got.Equal(want) || got.Location() != time.UTC { + t.Errorf("got %v (%v), want %v (UTC)", got, got.Location(), want) + } + }) +} + // ScanCell renders DATE / TIMESTAMP in the requested location, matching the // Thrift path's .In(location); a nil location leaves the value in UTC. func TestScanCellTimestampLocation(t *testing.T) { diff --git a/internal/rows/arrowbased/arrowRows.go b/internal/rows/arrowbased/arrowRows.go index 58970d85..50f9f402 100644 --- a/internal/rows/arrowbased/arrowRows.go +++ b/internal/rows/arrowbased/arrowRows.go @@ -651,6 +651,12 @@ func (vcm *arrowValueContainerMaker) makeColumnValueContainer(t arrow.DataType, case *arrow.TimestampType: return ×tampValueContainer{location: location, toTimestampFn: toTimestampFn}, nil + case *arrow.Time32Type: + return &timeValueContainer{nsPerUnit: timeUnitToNanos(t.Unit)}, nil + + case *arrow.Time64Type: + return &timeValueContainer{nsPerUnit: timeUnitToNanos(t.Unit)}, nil + case *arrow.BinaryType: return &columnValuesTyped[*array.Binary, []byte]{}, nil diff --git a/internal/rows/arrowbased/arrowscan_parity_test.go b/internal/rows/arrowbased/arrowscan_parity_test.go index a42856d2..7b50b802 100644 --- a/internal/rows/arrowbased/arrowscan_parity_test.go +++ b/internal/rows/arrowbased/arrowscan_parity_test.go @@ -169,6 +169,23 @@ func TestArrowbasedKernelRenderParity(t *testing.T) { b.FieldBuilder(0).(*array.TimestampBuilder).Append(arrow.Timestamp(ts.UnixMicro())) return b.NewArray() }}, + // Nested SQL TIME leaf: both backends must emit the zoneless time-of-day + // string ("09:15:30"), NOT a quoted time.Time .String() that would leak + // the 1970-01-01 epoch anchor date. time32 is seconds-unit here. + {"nested_time32", func() arrow.Array { + dt := arrow.StructOf(arrow.Field{Name: "t", Type: arrow.FixedWidthTypes.Time32s}) + b := array.NewStructBuilder(pool, dt) + b.Append(true) + b.FieldBuilder(0).(*array.Time32Builder).Append(arrow.Time32(9*3600 + 15*60 + 30)) + return b.NewArray() + }}, + {"nested_time64", func() arrow.Array { + dt := arrow.StructOf(arrow.Field{Name: "t", Type: arrow.FixedWidthTypes.Time64us}) + b := array.NewStructBuilder(pool, dt) + b.Append(true) + b.FieldBuilder(0).(*array.Time64Builder).Append(arrow.Time64((9*3600+15*60+30)*1_000_000 + 123_456)) + return b.NewArray() + }}, // VARIANT and GEOMETRY arrive over Arrow as plain STRING columns (verified // live: a top-level VARIANT is the JSON *string* "{\"a\":1}", GEOMETRY is the // WKT string "POINT(1 2)"). Nested inside a container they are string leaves, @@ -385,6 +402,16 @@ func TestArrowbasedKernelTopLevelScalarParity(t *testing.T) { b.Append(arrow.Date32FromTime(time.Date(2026, 7, 9, 0, 0, 0, 0, time.UTC))) return b.NewArray() }}, + {"time32_millis", func() arrow.Array { + b := array.NewTime32Builder(pool, &arrow.Time32Type{Unit: arrow.Millisecond}) + b.Append(arrow.Time32((13*3600+1*60+1)*1000 + 500)) // 13:01:01.500 + return b.NewArray() + }}, + {"time64_micros", func() arrow.Array { + b := array.NewTime64Builder(pool, &arrow.Time64Type{Unit: arrow.Microsecond}) + b.Append(arrow.Time64((13*3600 + 1*60 + 1) * 1_000_000)) // 13:01:01 + return b.NewArray() + }}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/rows/arrowbased/columnValues.go b/internal/rows/arrowbased/columnValues.go index 73aeb01f..5b7893b5 100644 --- a/internal/rows/arrowbased/columnValues.go +++ b/internal/rows/arrowbased/columnValues.go @@ -473,6 +473,92 @@ func (dvc *dateValueContainer) SetValueArray(colData arrow.ArrayData) error { return nil } +// timeUnitToNanos converts an Arrow time unit to its length in nanoseconds. +func timeUnitToNanos(unit arrow.TimeUnit) int64 { + switch unit { + case arrow.Second: + return int64(time.Second) + case arrow.Millisecond: + return int64(time.Millisecond) + case arrow.Microsecond: + return int64(time.Microsecond) + default: // arrow.Nanosecond + return 1 + } +} + +// timeValueContainer scans an Arrow time32/time64 column (a SQL TIME value, +// i.e. a time-of-day with no calendar date and no timezone). The Arrow value +// counts time units since midnight; we render it as a wall-clock time.Time +// anchored on the Unix epoch date in UTC so Hour/Minute/Second/Nanosecond read +// back correctly without introducing a spurious date or location shift. +type timeValueContainer struct { + timeArray arrow.Array + nsPerUnit int64 +} + +var _ columnValues = (*timeValueContainer)(nil) + +func (tvc *timeValueContainer) Value(i int) (any, error) { + var unitsSinceMidnight int64 + switch a := tvc.timeArray.(type) { + case *array.Time32: + unitsSinceMidnight = int64(a.Value(i)) + case *array.Time64: + unitsSinceMidnight = int64(a.Value(i)) + default: + return nil, errors.New(errArrowRowsUnhandledArrowType(tvc.timeArray.DataType().String())) + } + + ns := unitsSinceMidnight * tvc.nsPerUnit + // A SQL TIME is a zoneless wall-clock time-of-day, so we keep the value in + // UTC and do NOT apply cfg.Location: converting into a non-UTC location + // would shift the hour/minute (and could roll the anchor date off the + // epoch), which is wrong for a value that carries no timezone. + val := time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC). + Add(time.Duration(ns)) + return val, nil +} + +// ValueString renders the TIME as a zoneless time-of-day string (HH:MM:SS +// with optional fractional seconds). It is used when a TIME appears nested +// inside a complex type (STRUCT/LIST/MAP): the default time.Time marshaling +// emits t.String(), which would leak the spurious 1970-01-01 epoch anchor +// date for a value that carries no calendar date. See marshalScalar. +func (tvc *timeValueContainer) ValueString(i int) string { + v, err := tvc.Value(i) + if err != nil { + return "" + } + t, ok := v.(time.Time) + if !ok { + return "" + } + return t.Format("15:04:05.999999999") +} + +func (tvc *timeValueContainer) IsNull(i int) bool { + return tvc.timeArray.IsNull(i) +} + +func (tvc *timeValueContainer) Release() { + if tvc.timeArray != nil { + tvc.timeArray.Release() + } +} + +func (tvc *timeValueContainer) SetValueArray(colData arrow.ArrayData) error { + switch colData.DataType().(type) { + case *arrow.Time32Type: + tvc.timeArray = array.NewTime32Data(colData) + case *arrow.Time64Type: + tvc.timeArray = array.NewTime64Data(colData) + default: + return errors.New(errArrowRowsUnhandledArrowType(colData.DataType().String())) + } + return nil +} + type timestampValueContainer struct { timeArray *array.Timestamp location *time.Location @@ -597,6 +683,13 @@ func marshalScalar(holder columnValues, i int, val any) ([]byte, error) { if d, ok := holder.(*decimal128Container); ok { return []byte(d.ValueString(i)), nil } + // A SQL TIME is a zoneless time-of-day. Rendering it via the generic + // time.Time path would emit t.String() with the 1970-01-01 epoch anchor + // date (e.g. "1970-01-01 09:15:00 +0000 UTC"), leaking a spurious date for + // a value that carries none. Emit just the time-of-day as a JSON string. + if t, ok := holder.(*timeValueContainer); ok { + return json.Marshal(t.ValueString(i)) + } return marshal(val) } diff --git a/internal/rows/arrowbased/time_test.go b/internal/rows/arrowbased/time_test.go new file mode 100644 index 00000000..88a85500 --- /dev/null +++ b/internal/rows/arrowbased/time_test.go @@ -0,0 +1,175 @@ +package arrowbased + +import ( + "testing" + "time" + + "github.com/apache/arrow/go/v12/arrow" + "github.com/apache/arrow/go/v12/arrow/array" + "github.com/apache/arrow/go/v12/arrow/memory" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// buildTime64Array builds a single-element arrow Time64 array in the given unit +// holding the provided raw value (value since midnight in that unit). +func buildTime64Array(t *testing.T, unit arrow.TimeUnit, v arrow.Time64) arrow.ArrayData { + t.Helper() + dt := &arrow.Time64Type{Unit: unit} + b := array.NewTime64Builder(memory.DefaultAllocator, dt) + defer b.Release() + b.Append(v) + arr := b.NewArray() + defer arr.Release() + arr.Data().Retain() + return arr.Data() +} + +// buildTime32Array builds a single-element arrow Time32 array in the given unit. +func buildTime32Array(t *testing.T, unit arrow.TimeUnit, v arrow.Time32) arrow.ArrayData { + t.Helper() + dt := &arrow.Time32Type{Unit: unit} + b := array.NewTime32Builder(memory.DefaultAllocator, dt) + defer b.Release() + b.Append(v) + arr := b.NewArray() + defer arr.Release() + arr.Data().Retain() + return arr.Data() +} + +// TestTime64ValueContainer guards the time64 -> time-of-day conversion for both +// nanosecond and microsecond Arrow units. A TIME '09:15:00.123456' arrives as a +// time64 value counting units since midnight; the scanned value must be a +// wall-clock time-of-day (Hour/Minute/Second/Nanosecond) in UTC, anchored on the +// epoch date (no meaningful calendar date, no location shift). +func TestTime64ValueContainer(t *testing.T) { + vcm := &arrowValueContainerMaker{} + + // nonUTC is a fixed positive-offset location used to prove that a zoneless + // SQL TIME is never shifted by cfg.Location (a positive offset would also + // roll the epoch anchor date backwards if the value were converted). + nonUTC := time.FixedZone("UTC+5", 5*3600) + + cases := []struct { + name string + unit arrow.TimeUnit + raw arrow.Time64 + loc *time.Location + wantNs int + }{ + { + name: "nanosecond", + unit: arrow.Nanosecond, + // 09:15:00.123456 -> (9*3600+15*60)*1e9 + 123456000 ns since midnight + raw: arrow.Time64((9*3600+15*60)*int64(time.Second) + 123456000), + loc: time.UTC, + wantNs: 123456000, + }, + { + name: "microsecond", + unit: arrow.Microsecond, + // 09:15:00.123456 -> (9*3600+15*60)*1e6 + 123456 us since midnight + raw: arrow.Time64((9*3600+15*60)*int64(time.Second/time.Microsecond) + 123456), + loc: time.UTC, + wantNs: 123456000, + }, + { + // A non-UTC cfg.Location must NOT shift a zoneless TIME: the hour + // stays 09 and the value stays on the UTC epoch date. + name: "nanosecond non-UTC location", + unit: arrow.Nanosecond, + raw: arrow.Time64((9*3600+15*60)*int64(time.Second) + 123456000), + loc: nonUTC, + wantNs: 123456000, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dt := &arrow.Time64Type{Unit: tc.unit} + holder, err := vcm.makeColumnValueContainer(dt, tc.loc, nil, nil) + require.NoError(t, err) + + data := buildTime64Array(t, tc.unit, tc.raw) + defer data.Release() + require.NoError(t, holder.SetValueArray(data)) + + v, err := holder.Value(0) + require.NoError(t, err) + + tv, ok := v.(time.Time) + require.True(t, ok, "expected time.Time, got %T", v) + + assert.Equal(t, 9, tv.Hour()) + assert.Equal(t, 15, tv.Minute()) + assert.Equal(t, 0, tv.Second()) + assert.Equal(t, tc.wantNs, tv.Nanosecond()) + assert.Equal(t, time.UTC, tv.Location()) + }) + } +} + +// TestTime32ValueContainer is the defensive edge: a TIME carried as Arrow time32 +// (second / millisecond units) must also scan as a wall-clock time-of-day rather +// than hitting the unhandled-type error. +func TestTime32ValueContainer(t *testing.T) { + vcm := &arrowValueContainerMaker{} + + nonUTC := time.FixedZone("UTC+5", 5*3600) + + cases := []struct { + name string + unit arrow.TimeUnit + raw arrow.Time32 + loc *time.Location + wantNs int + }{ + { + name: "second", + unit: arrow.Second, + raw: arrow.Time32(9*3600 + 15*60), + loc: time.UTC, + wantNs: 0, + }, + { + name: "millisecond", + unit: arrow.Millisecond, + raw: arrow.Time32((9*3600+15*60)*1000 + 123), + loc: time.UTC, + wantNs: 123000000, + }, + { + // A non-UTC cfg.Location must NOT shift a zoneless TIME. + name: "second non-UTC location", + unit: arrow.Second, + raw: arrow.Time32(9*3600 + 15*60), + loc: nonUTC, + wantNs: 0, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dt := &arrow.Time32Type{Unit: tc.unit} + holder, err := vcm.makeColumnValueContainer(dt, tc.loc, nil, nil) + require.NoError(t, err) + + data := buildTime32Array(t, tc.unit, tc.raw) + defer data.Release() + require.NoError(t, holder.SetValueArray(data)) + + v, err := holder.Value(0) + require.NoError(t, err) + + tv, ok := v.(time.Time) + require.True(t, ok, "expected time.Time, got %T", v) + + assert.Equal(t, 9, tv.Hour()) + assert.Equal(t, 15, tv.Minute()) + assert.Equal(t, 0, tv.Second()) + assert.Equal(t, tc.wantNs, tv.Nanosecond()) + assert.Equal(t, time.UTC, tv.Location()) + }) + } +}