CurtHagenlocher commented on code in PR #2917: URL: https://github.com/apache/arrow-adbc/pull/2917#discussion_r2123937244
########## docs/source/driver/snowflake.rst: ########## @@ -469,6 +469,10 @@ These options map 1:1 with the Snowflake `Config object <https://pkg.go.dev/gith non-zero scaled columns will be returned as ``Float64`` typed Arrow columns. The default is ``true``. +``adbc.snowflake.sql.client_option.use_max_microseconds_precision`` + When ``true``, nanoseconds will be converted to microseconds + to avoid the overflow of the timestamp type. Review Comment: These two lines use tabs where everything else in this file is spaces. I suspect this is the cause of the checkin test failure. ########## go/adbc/driver/snowflake/connection.go: ########## @@ -483,9 +484,17 @@ func (c *connectionImpl) toArrowField(columnInfo driverbase.ColumnInfo) arrow.Fi case "DATETIME": fallthrough case "TIMESTAMP", "TIMESTAMP_NTZ": - field.Type = &arrow.TimestampType{Unit: arrow.Nanosecond} + if c.useMaxMicrosecondTimestampPrecision { Review Comment: There's similar code in this file in the function `descToField`. That doesn't need updating? ########## docs/source/driver/snowflake.rst: ########## @@ -469,6 +469,10 @@ These options map 1:1 with the Snowflake `Config object <https://pkg.go.dev/gith non-zero scaled columns will be returned as ``Float64`` typed Arrow columns. The default is ``true``. +``adbc.snowflake.sql.client_option.use_max_microseconds_precision`` + When ``true``, nanoseconds will be converted to microseconds + to avoid the overflow of the timestamp type. Review Comment: This does not affect time values, only timestamp values. It might be worth calling that out here in the documentation. ########## go/adbc/driver/snowflake/driver_test.go: ########## @@ -1697,6 +1698,85 @@ func (suite *SnowflakeTests) TestBooleanType() { } } +func (suite *SnowflakeTests) TestTimestampPrecision() { + + // with max microseconds precision + rec := suite.getTimestamps(true, false) + + v1, _ := arrow.TimestampFromTime(time.Date(1, 1, 1, 0, 0, 0, 0, time.UTC), arrow.Microsecond) + v2, _ := arrow.TimestampFromTime(time.Date(2025, 6, 2, 10, 37, 56, 123456789, time.UTC), arrow.Microsecond) + v3, _ := arrow.TimestampFromTime(time.Date(9999, 12, 31, 23, 59, 59, 999999999, time.UTC), arrow.Microsecond) + + // Expected values + expectedMicroseconds := []arrow.Timestamp{ + v1, + v2, + v3, + } + + suite.validateTimestamps(rec, expectedMicroseconds) + + // with the default nanoseconds precision + rec = suite.getTimestamps(false, false) + + expectedOverflowValues := []arrow.Timestamp{ + //overflows to August 30, 1754 at 22:43:41.128654 UTC. + arrow.Timestamp(time.Date(1, 1, 1, 0, 0, 0, 0, time.UTC).UnixNano()), + arrow.Timestamp(time.Date(2025, 6, 2, 10, 37, 56, 123456789, time.UTC).UnixNano()), + //overflows to March 30, 1816 at 05:56:08.066278 UTC. + arrow.Timestamp(time.Date(9999, 12, 31, 23, 59, 59, 999999999, time.UTC).UnixNano()), + } + + suite.validateTimestamps(rec, expectedOverflowValues) + + // just by setting the statement option + rec = suite.getTimestamps(false, true) + suite.validateTimestamps(rec, expectedMicroseconds) + + // set the database option to microseconds but then disable the statement option + rec = suite.getTimestamps(true, false) + suite.validateTimestamps(rec, expectedOverflowValues) +} + +func (suite *SnowflakeTests) getTimestamps(setDatabaseOption bool, setStatementOption bool) arrow.Record { + query := "select TO_TIMESTAMP('0001-01-01 00:00:00.000000000') as Jan01_0001, TO_TIMESTAMP('2025-06-02 10:37:56.123456789') as June02_2025, TO_TIMESTAMP('9999-12-31 23:59:59.999999999') As December31_9999" Review Comment: The Snowflake documentation says that `TO_TIMESTAMP` maps to either `TO_TIMESTAMP_LTZ`, `TO_TIMESTAMP_NTZ` or `TO_TIMESTAMP_TZ` based on a session setting. Rather than testing via `TO_TIMESTAMP`, it might be better to individually test all three of those functions. ########## go/adbc/driver/snowflake/connection.go: ########## @@ -483,9 +484,17 @@ func (c *connectionImpl) toArrowField(columnInfo driverbase.ColumnInfo) arrow.Fi case "DATETIME": fallthrough case "TIMESTAMP", "TIMESTAMP_NTZ": - field.Type = &arrow.TimestampType{Unit: arrow.Nanosecond} + if c.useMaxMicrosecondTimestampPrecision { + field.Type = &arrow.TimestampType{Unit: arrow.Microsecond} + } else { + field.Type = &arrow.TimestampType{Unit: arrow.Nanosecond} + } case "TIMESTAMP_LTZ": - field.Type = &arrow.TimestampType{Unit: arrow.Nanosecond, TimeZone: loc.String()} + if c.useMaxMicrosecondTimestampPrecision { + field.Type = &arrow.TimestampType{Unit: arrow.Microsecond, TimeZone: loc.String()} + } else { + field.Type = &arrow.TimestampType{Unit: arrow.Nanosecond, TimeZone: loc.String()} + } case "TIMESTAMP_TZ": field.Type = arrow.FixedWidthTypes.Timestamp_ns Review Comment: This is also a timestamp with nanoseconds. Shouldn't this also get a conversion to microseconds? ########## csharp/test/Drivers/Interop/Snowflake/SnowflakeTestConfiguration.cs: ########## @@ -73,11 +73,17 @@ internal class SnowflakeTestConfiguration : TestConfiguration public string Warehouse { get; set; } = string.Empty; /// <summary> - /// The Snowflake use high precision + /// The Snowflake setting to use high precision /// </summary> [JsonPropertyName("useHighPrecision")] public bool UseHighPrecision { get; set; } = true; + /// <summary> + /// The Snowflake setting to only have a max timestamp precision of microseconds Review Comment: ```suggestion /// The Snowflake setting to have a max timestamp precision of only microseconds ``` -- 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]
