Skip to content
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

feat: redshift data driver #18

Merged
merged 3 commits into from
Mar 26, 2024
Merged

feat: redshift data driver #18

merged 3 commits into from
Mar 26, 2024

Conversation

atzoum
Copy link
Collaborator

@atzoum atzoum commented Mar 14, 2024

Description

Including the option to use an alternative sql driver for redshift, based on the Redshift Data API Service.

Credentials example for using the new redshift-data driver:

{
    "type": "redshift-data", <-- type qualifier
    "clusterIdentifier": "clusterIdentifier",
    "database": "database",
    "user": "user",
    "region": "region",
    "sharedConfigProfile": "sharedConfigProfile"
}

Credentials example for using the original postgres driver:

{
    "host": "host",
    "port": 5432,
    "dbname": "dbname",
    "user": "user",
    "password": "password",
    "sslmode": "sslmode"
}

The sqlconnect.DB interface's behaviour remains the same regardless which driver is being used, in terms of:

  • Column mappings
  • Json mappings
  • SQL dialect support & parameterized queries

However, differences shall be expected by clients in the following:

  • Prepared statements: the driver doesn't actually prepare any statement against the data API, it merely caches it in memory, until the caller decides to execute it.
  • Transactions: the Data API doesn't support transactions natively, it only supports batch execution of statements. Driver's transactions are implemented on top of the batch execution API, therefore only ExecContext operations are allowed in the context of an ongoing transaction, not QueryContext.

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@@ -8,9 +8,9 @@
"_integer": 1,
"_smallint": 1,
"_bigint": 1,
"_real": 1.1,
Copy link
Collaborator Author

@atzoum atzoum Mar 14, 2024

Choose a reason for hiding this comment

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

Note: redshift-data driver returns some imprecise value, e.g. 1.10000000034

"_float": 1.1,
"_float4": 1.1,
"_float4": 1,
Copy link
Collaborator Author

@atzoum atzoum Mar 14, 2024

Choose a reason for hiding this comment

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

Note: redshift-data driver returns some imprecise value, e.g. 1.10000000034

@@ -44,6 +44,10 @@ func jsonRowMapper(databaseTypeName string, value any) any {
if n, err := strconv.ParseFloat(string(v), 64); err == nil {
return n
}
case string:
Copy link
Collaborator Author

@atzoum atzoum Mar 14, 2024

Choose a reason for hiding this comment

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

Note: redshift-data driver returns NUMERIC data types as string instead of []byte

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 75.22124% with 140 lines in your changes are missing coverage. Please review.

Project coverage is 85.84%. Comparing base (87f2a09) to head (92b401f).

Files Patch % Lines
sqlconnect/internal/redshift/driver/connection.go 64.97% 48 Missing and 28 partials ⚠️
sqlconnect/internal/redshift/driver/dsn.go 84.56% 13 Missing and 10 partials ⚠️
sqlconnect/internal/redshift/driver/rows.go 74.24% 11 Missing and 6 partials ⚠️
sqlconnect/internal/redshift/config.go 60.00% 2 Missing and 2 partials ⚠️
sqlconnect/internal/redshift/driver/driver.go 73.33% 2 Missing and 2 partials ⚠️
sqlconnect/internal/redshift/driver/result.go 77.77% 2 Missing and 2 partials ⚠️
sqlconnect/internal/redshift/driver/statement.go 66.66% 4 Missing ⚠️
sqlconnect/internal/redshift/db.go 95.12% 1 Missing and 1 partial ⚠️
sqlconnect/internal/redshift/driver/client.go 66.66% 1 Missing and 1 partial ⚠️
sqlconnect/internal/redshift/driver/connector.go 81.81% 1 Missing and 1 partial ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #18      +/-   ##
==========================================
- Coverage   88.33%   85.84%   -2.50%     
==========================================
  Files          59       70      +11     
  Lines        2143     2684     +541     
==========================================
+ Hits         1893     2304     +411     
- Misses        173      251      +78     
- Partials       77      129      +52     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dest[i] = nil
case *types.FieldMemberStringValue:
switch {
case strings.EqualFold(*rows.page.ColumnMetadata[i].TypeName, "timestamp"):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: to be compatible with the behaviour of pg driver, we need to:

  • Parse the string into a time
  • Convert the time to UTC and parse it as an RFC3339 string.

@atzoum atzoum force-pushed the feat.redshiftDriver branch from 029e9d3 to 606462d Compare March 14, 2024 16:39
@atzoum atzoum requested a review from kuldeep0020 March 14, 2024 16:39
@atzoum atzoum force-pushed the feat.redshiftDriver branch 4 times, most recently from c4a38b3 to b6f691a Compare March 20, 2024 08:16
@atzoum atzoum changed the title [WIP] feat: redshift sdk driver feat: redshift sdk driver Mar 20, 2024
@atzoum atzoum marked this pull request as ready for review March 20, 2024 08:27
@atzoum atzoum force-pushed the feat.redshiftDriver branch 5 times, most recently from d0b611e to 2073245 Compare March 22, 2024 08:16
@atzoum atzoum force-pushed the feat.redshiftDriver branch from 2073245 to 63020ec Compare March 22, 2024 08:17
@atzoum atzoum requested a review from nishantsharma March 22, 2024 08:34
@atzoum atzoum force-pushed the feat.redshiftDriver branch from 63020ec to 1eadb3e Compare March 22, 2024 12:23
@atzoum atzoum requested a review from achettyiitr March 22, 2024 12:25
@atzoum atzoum changed the title feat: redshift sdk driver feat: redshift data api driver Mar 22, 2024
@atzoum atzoum changed the title feat: redshift data api driver feat: redshift data driver Mar 22, 2024
@atzoum atzoum force-pushed the feat.redshiftDriver branch 3 times, most recently from 307c3e5 to a878fda Compare March 22, 2024 16:19
@atzoum atzoum force-pushed the feat.redshiftDriver branch from a878fda to 92b401f Compare March 22, 2024 16:24
@atzoum atzoum merged commit 6555d97 into main Mar 26, 2024
18 checks passed
@atzoum atzoum deleted the feat.redshiftDriver branch March 26, 2024 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant