Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions go/driver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2306,11 +2306,6 @@ func (suite *SnowflakeTests) TestIntDecimalLowPrecision() {
numberString = "-" + numberString
}
query := "SELECT CAST('" + numberString + fmt.Sprintf("' AS NUMBER(%d, %d)) AS RESULT", precision, scale)
decimalNumber, err := decimal128.FromString(numberString, int32(precision), int32(scale))
suite.NoError(err)
// The current behavior of the driver for decimal128 values too large to fit into 64 bits is to simply
// return the low 64 bits of the value.
number := int64(decimalNumber.LowBits())

suite.Require().NoError(suite.stmt.SetOption(suite.ctx, driver.OptionUseHighPrecision, adbc.OptionValueDisabled))
suite.Require().NoError(suite.stmt.SetSqlQuery(suite.ctx, query))
Expand All @@ -2320,11 +2315,23 @@ func (suite *SnowflakeTests) TestIntDecimalLowPrecision() {

suite.EqualValues(1, n)
suite.Truef(arrow.TypeEqual(arrow.PrimitiveTypes.Int64, rdr.Schema().Field(0).Type), "expected int64, got %s", rdr.Schema().Field(0).Type)
suite.True(rdr.Next())
rec := rdr.RecordBatch()

value := rec.Column(0).(*array.Int64).Value(0)
suite.Equal(number, value)
// An all-nines value only fits in int64 up to precision 18; beyond that
// it exceeds the int64 range and, with use_high_precision=false, the
// driver now reports an error rather than truncating to the low 64 bits.
if precision <= 18 {
decimalNumber, err := decimal128.FromString(numberString, int32(precision), int32(scale))
suite.Require().NoError(err)
suite.True(rdr.Next())
rec := rdr.RecordBatch()
value := rec.Column(0).(*array.Int64).Value(0)
suite.Equal(int64(decimalNumber.LowBits()), value)
} else {
suite.False(rdr.Next())
var adbcErr adbc.Error
suite.Require().ErrorAs(rdr.Err(), &adbcErr)
suite.Equal(adbc.StatusInvalidData, adbcErr.Code)
}
}
}
}
Expand Down
75 changes: 71 additions & 4 deletions go/record_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import (
"github.com/apache/arrow-go/v18/arrow"
"github.com/apache/arrow-go/v18/arrow/array"
"github.com/apache/arrow-go/v18/arrow/compute"
"github.com/apache/arrow-go/v18/arrow/decimal"
"github.com/apache/arrow-go/v18/arrow/ipc"
"github.com/apache/arrow-go/v18/arrow/memory"
"github.com/snowflakedb/gosnowflake/v2"
Expand Down Expand Up @@ -259,6 +260,10 @@ func getRecTransformer(sc *arrow.Schema, tr []colTransformer) recordTransformer

for i, col := range r.Columns() {
if cols[i], err = tr[i](ctx, col); err != nil {
var adbcErr adbc.Error
if errors.As(err, &adbcErr) {
return nil, adbcErr
}
return nil, errToAdbcErr(adbc.StatusInternal, err)
}
}
Expand Down Expand Up @@ -312,12 +317,13 @@ func getTransformer(sc *arrow.Schema, ld gosnowflake.ArrowStreamLoader, useHighP
} else {
if srcMeta.Scale == 0 {
f.Type = arrow.PrimitiveTypes.Int64
transformers[i] = decimalScale0ToInt64(f.Name)
} else {
f.Type = arrow.PrimitiveTypes.Float64
}
dt := f.Type
transformers[i] = func(ctx context.Context, a arrow.Array) (arrow.Array, error) {
return compute.CastArray(ctx, a, compute.UnsafeCastOptions(dt))
dt := f.Type
transformers[i] = func(ctx context.Context, a arrow.Array) (arrow.Array, error) {
return compute.CastArray(ctx, a, compute.UnsafeCastOptions(dt))
}
}
}
default:
Expand Down Expand Up @@ -565,6 +571,67 @@ func scaledIntToFloat64Transformer(precision, scale int32) colTransformer {
}
}

// decimalScale0ToInt64 converts a scale-0 Decimal128/Decimal256 column to int64
// for use_high_precision=false, returning a StatusInvalidData error for any value
// outside the int64 range instead of silently truncating it (issue #129).
//
// The range is checked manually rather than with compute.SafeCastOptions because
// arrow-go's safe decimal->int kernel tests `value >= MaxInt64` and so rejects
// math.MaxInt64 itself; a manual check accepts both int64 bounds inclusively.
Comment on lines +578 to +580

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Er, should we fix this upstream?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good point. don't know why i didn't just do that when i saw this...

func decimalScale0ToInt64(colName string) colTransformer {
rangeErr := adbc.Error{
Code: adbc.StatusInvalidData,
Msg: fmt.Sprintf("[snowflake] column %q contains a NUMBER value outside the int64 range; "+
"set %s=true to read it losslessly as decimal128", colName, OptionUseHighPrecision),
}
min128, errMin128 := decimal.Decimal128FromString("-9223372036854775808", 19, 0)
max128, errMax128 := decimal.Decimal128FromString("9223372036854775807", 19, 0)
min256, errMin256 := decimal.Decimal256FromString("-9223372036854775808", 19, 0)
max256, errMax256 := decimal.Decimal256FromString("9223372036854775807", 19, 0)
if err := errors.Join(errMin128, errMax128, errMin256, errMax256); err != nil {
return func(context.Context, arrow.Array) (arrow.Array, error) {
return nil, errToAdbcErr(adbc.StatusInternal, err)
}
}
return func(ctx context.Context, a arrow.Array) (arrow.Array, error) {
bldr := array.NewInt64Builder(compute.GetAllocator(ctx))
defer bldr.Release()
bldr.Reserve(a.Len())
switch arr := a.(type) {
case *array.Decimal128:
for i := 0; i < arr.Len(); i++ {
if arr.IsNull(i) {
bldr.AppendNull()
continue
}
v := arr.Value(i)
if v.Less(min128) || max128.Less(v) {
return nil, rangeErr
}
bldr.Append(int64(v.LowBits()))
}
case *array.Decimal256:
for i := 0; i < arr.Len(); i++ {
if arr.IsNull(i) {
bldr.AppendNull()
continue
}
v := arr.Value(i)
if v.Less(min256) || max256.Less(v) {
return nil, rangeErr
}
bldr.Append(int64(v.LowBits()))
}
default:
return nil, adbc.Error{
Code: adbc.StatusInternal,
Msg: fmt.Sprintf("[snowflake] column %q: expected decimal array for scale-0 NUMBER, got %s", colName, a.DataType()),
}
}
return bldr.NewArray(), nil
}
}

func integerToDecimal128(ctx context.Context, a arrow.Array, dt *arrow.Decimal128Type) (arrow.Array, error) {
// We can't do a cast directly into the destination type because the numbers we get from Snowflake
// are scaled integers. So not only would the cast produce the wrong value, it also risks producing
Expand Down
55 changes: 55 additions & 0 deletions go/record_reader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,17 @@ import (
"context"
"fmt"
"io"
"math"
"strings"
"sync"
"testing"
"time"

"github.com/apache/arrow-adbc/go/adbc"
"github.com/apache/arrow-go/v18/arrow"
"github.com/apache/arrow-go/v18/arrow/array"
"github.com/apache/arrow-go/v18/arrow/compute"
"github.com/apache/arrow-go/v18/arrow/decimal"
"github.com/apache/arrow-go/v18/arrow/ipc"
"github.com/apache/arrow-go/v18/arrow/memory"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -477,6 +481,57 @@ func TestFixedToFloat64Transformer(t *testing.T) {
}
}

func TestDecimalScale0ToInt64(t *testing.T) {
cases := []struct {
name string
value string
wantErr bool
want int64
}{
{name: "zero", value: "0", want: 0},
{name: "positive", value: "1", want: 1},
{name: "negative", value: "-1", want: -1},
{name: "int64Max", value: "9223372036854775807", want: math.MaxInt64},
{name: "int64Min", value: "-9223372036854775808", want: math.MinInt64},
{name: "int64MaxPlusOne", value: "9223372036854775808", wantErr: true},
{name: "int64MinMinusOne", value: "-9223372036854775809", wantErr: true},
{name: "farOverflow", value: "12345678901234567890123456789012345678", wantErr: true},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
alloc := memory.NewCheckedAllocator(memory.DefaultAllocator)
defer alloc.AssertSize(t, 0)

bldr := array.NewDecimal128Builder(alloc, &arrow.Decimal128Type{Precision: 38, Scale: 0})
defer bldr.Release()
v, err := decimal.Decimal128FromString(tc.value, 38, 0)
require.NoError(t, err)
bldr.Append(v)
bldr.AppendNull()
arr := bldr.NewArray()
defer arr.Release()

ctx := compute.WithAllocator(context.Background(), alloc)
out, err := decimalScale0ToInt64("col")(ctx, arr)
if tc.wantErr {
var adbcErr adbc.Error
require.ErrorAs(t, err, &adbcErr)
assert.Equal(t, adbc.StatusInvalidData, adbcErr.Code)
assert.Nil(t, out)
return
}

require.NoError(t, err)
defer out.Release()
require.IsType(t, (*array.Int64)(nil), out)
result := out.(*array.Int64)
require.Equal(t, 2, result.Len())
assert.Equal(t, tc.want, result.Value(0))
assert.True(t, result.IsNull(1))
})
}
}

func TestReadBatchRecords_RetriesAfterRowCountMismatch(t *testing.T) {
alloc := memory.NewCheckedAllocator(memory.DefaultAllocator)
defer alloc.AssertSize(t, 0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,5 @@ drop = "test_numeric_low_precision"
[tags]
sql-type-name = "NUMERIC (use_high_precision = false)"
caveats = [
"When use_high_precision=false, values exceeding int64 range are silently truncated — the driver discards the upper bits and returns only the lower 64 bits without warning."
"When use_high_precision=false, NUMBER values outside the int64 range cannot be represented as int64, so the driver returns an error instead of a value. Set use_high_precision=true to read such columns losslessly as decimal128."
]
Loading