-
Notifications
You must be signed in to change notification settings - Fork 163
fix(go/adbc/driver/snowflake): Adjust the precision of the Timestamp values in the JSON-only path #2965
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(go/adbc/driver/snowflake): Adjust the precision of the Timestamp values in the JSON-only path #2965
Conversation
CurtHagenlocher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should have a test in Go. That probably requires creating a table that has a primary key? There should be existing tests for returning a schema whose setup can be reused for that, I think.
csharp/test/Apache.Arrow.Adbc.Tests/ColumnNetTypeArrowTypeValue.cs
Outdated
Show resolved
Hide resolved
I started in Go and deleted it because it was specific to a single instance. I don’t think it would be too hard to do in Go what exists in C#. |
Actually, the problem doesn't occur in Go. The problem was happening because the schema was reported by the Go driver as microseconds and the value was in nanoseconds, so when the IArrowArray.ValueAt method was called, it was failing on the internal ValueConverter at: which goes to the internal TimstampArray here: All of that to say, I don't know what a separate Go test will really do for that scenario. |
|
I'm not sure how it can be said that the problem doesn't exist in Go; if the fix is in the driver then the problem is in the driver. If the schema reported by the Arrow array stream is different than the schema reported by the record batch, then that's the bug and it can be tested for in Go. |
CurtHagenlocher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
|
Yes, but there is only one row in the result.
________________________________
From: David Li ***@***.***>
Sent: Sunday, June 15, 2025 2:20:04 AM
To: apache/arrow-adbc ***@***.***>
Cc: David Coe ***@***.***>; Author ***@***.***>
Subject: Re: [apache/arrow-adbc] fix(go/adbc/driver/snowflake): Adjust the precision of the Timestamp values in the JSON-only path (PR #2965)
@lidavidm commented on this pull request.
________________________________
In go/adbc/driver/snowflake/driver_test.go<#2965 (comment)>:
+ numRows := int(rec.NumRows())
+ for i := 0; i < numRows; i++ {
+ dbName := rec.Column(dbIdx).(*array.String).Value(i)
+ schema := rec.Column(schemaIdx).(*array.String).Value(i)
+ tbl := rec.Column(tableIdx).(*array.String).Value(i)
+ column := rec.Column(colIdx).(*array.String).Value(i)
+ keySeq := rec.Column(seqIdx).(*array.Int64).Value(i)
+
+ // Created_on should be a timestamp array
+ createdCol := rec.Column(createdIdx).(*array.Timestamp)
+ created := time.Unix(0, int64(createdCol.Value(i))*int64(time.Microsecond)).UTC()
+
+ // Perform checks
+ if strings.EqualFold(dbName, catalogName) &&
+ strings.EqualFold(schema, schemaName) &&
+ strings.EqualFold(tbl, tableName) &&
+ strings.EqualFold(column, "id") &&
+ keySeq == 1 {
+
+ now := time.Now().UTC()
+ diff := now.Sub(created)
+ if diff < 0 {
+ diff = -diff
+ }
+ if diff <= 2*time.Minute {
+ return true
+ }
+ }
+ }
+
+ return false
Is it intended for this loop to stop once it finds one matching row?
—
Reply to this email directly, view it on GitHub<#2965 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ADFTVNOQSUARO37AOZN6NKT3DUGBJAVCNFSM6AAAAAB7I66K46VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSMRYHE4TONBZGQ>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
If there's only one row in the result, can we assert that fact? The loop is rather misleading in this case. |
| suite.True(rdr.Next()) | ||
| rec := rdr.Record() | ||
|
|
||
| suite.True(rec.NumRows() == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suite.Equal?
| seqIdx := getColIdx("key_sequence") | ||
| createdIdx := getColIdx("created_on") | ||
|
|
||
| if dbIdx == -1 || schemaIdx == -1 || tableIdx == -1 || colIdx == -1 || seqIdx == -1 || createdIdx == -1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suite.Equal instead of panic?
Or just panic inside of getColIndex with the column name.
| } | ||
|
|
||
| query = fmt.Sprintf("DROP TABLE %s.%s.%s", suite.Quirks.catalogName, suite.Quirks.schemaName, tempTable) | ||
| stmt, _ = cnxn.NewStatement() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't being cleaned up, though I suppose it doesn't seem to matter
| stmt, _ = cnxn.NewStatement() | ||
| suite.Require().NoError(stmt.SetSqlQuery(query)) | ||
| _, err = stmt.ExecuteUpdate(suite.ctx) | ||
| defer rdr.Release() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a double release? And why is it all the way down here?
After the update to adjust the Timestamp precision, the JSON path (which is hit when using SHOW commands) was still using nanoseconds even though the schema was specifying the precision as microseconds, resulting in downstream C# callers getting the error