feat: sqlconnect library #1
Conversation
atzoum
force-pushed
the
feat.sqlconnect
branch
2 times, most recently
from
aa5cfbe
to
79c255b
Compare
REV-779 sqlconnect-go library
Acceptance Criteria impact the following
Docs QA : Estimate (Weeks): Pair: None |
atzoum
force-pushed
the
feat.sqlconnect
branch
2 times, most recently
from
f1e070b
to
55718a9
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
atzoum
force-pushed
the
feat.sqlconnect
branch
23 times, most recently
from
c4d7d9a
to
b7d4e0b
Compare
| var val any | ||
| if v != nil { | ||
| val = v.Value | ||
| // copying bytes to avoid them being overwritten by the next row, since some drivers reuse the same buffer (e.g. postgres) |
There was a problem hiding this comment.
Note: this was a sneaky one... previously we were marshalling the json in the same goroutine so the issue was not possible to happen, whereas now, we are passing the map through the channel to another goroutine which could cause the map's contents to be overwritten in the meantime by the first goroutine, in case of []byte values.
There was a problem hiding this comment.
Wonder if integ tests helped in catching this bug. I had gone through this file, and indeed hard to catch 😨
There was a problem hiding this comment.
of course tests in rudder-sources helped, as a matter of fact all unexpected issues were revealed by those tests!
| if value == nil { | ||
| return nil | ||
| } | ||
| databaseTypeName = strings.Replace(databaseTypeName, "UNSIGNED ", "", 1) |
There was a problem hiding this comment.
Note: the new version of mysql driver, v1.7.1, identifies some numeric types as unsigned which would cause our mappings to behave differently if we kept them as they were, that's why we are removing the UNSIGNED prefix here.
| "CHAR": "string", | ||
| "VARCHAR": "string", |
There was a problem hiding this comment.
Note: we were previously using select * from table for listing a table's columns which always returned STRING for all string character types.
Now that we switched to describe table, we need to cater for CHAR and VARCHAR types in our mappings as well, which are supported by databricks but are undocumented...
| // Using a map[string]bigquery.Value instead of a []bigquery.Value for properly mapping structs. | ||
| // If we were to use a slice, structs would be mapped as an array of values, e.g. [value1, value2, ...] | ||
| // instead of {field1: value1, field2: value2, ...} | ||
| var values map[string]bigquery.Value |
There was a problem hiding this comment.
Note: this was indeed some unexpected behavior from bigquery's go client
| "DECIMAL": "int", | ||
| "NUMERIC": "int", |
There was a problem hiding this comment.
Decimal/Dec to int is a bug in legacy mapping? :(. Keeping a note to see if it can be fixed.
There was a problem hiding this comment.
yes, these are all legacy mappings, i.e. exact same behaviour as what rudder-sources is doing now
| "VOID": "string", | ||
| "TIMESTAMP": "datetime", |
There was a problem hiding this comment.
VOID datatype is interesting.
https://docs.databricks.com/en/sql/language-manual/data-types/null-type.html
sqlconnect/internal/integration_test/db_integration_test_scenario.go
Outdated
Show resolved
Hide resolved
| defer func() { | ||
| require.NoError(t, stmt.Close(), "it should be able to close the prepared statement") | ||
| }() | ||
| rows, err := stmt.Query(1) |
There was a problem hiding this comment.
In statement.go, Query func returns error driver.ErrSkip. For stmt.Query(1), is it that it ends up using QueryContext and thats how its working?
Description
Introducing sqlconnect library which provides a uniform client interface for accessing multiple warehouses:
Key features:
database/sqlsupport for BigQuery through a custom driver.useLegacyMappingsoption)Linear Ticket
resolves REV-779
Security