SNOW-1313648 GO - Verify value bindings for all field types while exceeding CLIENT_STAGE_ARRAY_BINDING_THRESHOLD #1297

sfc-gh-ext-simba-jy · 2025-01-31T19:49:03Z

Description

SNOW-313648 Please explain the changes you made here.

Fixed the time difference when the timestamp value is before 1600
Add testing to validate that inserting data with the stage binding, array binding, and interface array binding are the same.

Checklist

Created tests which fail without the change (if possible)
Extended the README / documentation, if necessary

codecov · 2025-02-05T00:25:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.19%. Comparing base (e926883) to head (8f8b1a4).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1297      +/-   ##
==========================================
- Coverage   82.20%   82.19%   -0.01%     
==========================================
  Files          55       55              
  Lines       13636    13657      +21     
==========================================
+ Hits        11209    11225      +16     
- Misses       2427     2432       +5

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

…lake into SNOW-1313648

sfc-gh-pfus · 2025-02-06T06:28:55Z

bindings_test.go

@@ -874,6 +874,132 @@ func TestBulkArrayBinding(t *testing.T) {
 	})
 }

+func TestSNOW1313648(t *testing.T) {


Can we have a better name?

sfc-gh-pfus · 2025-02-06T06:29:30Z

bindings_test.go

@@ -874,6 +874,132 @@ func TestBulkArrayBinding(t *testing.T) {
 	})
 }

+func TestSNOW1313648(t *testing.T) {
+	arrayInsertTable := "Snow1313648Insert"


Here the same, this is just a regular feature that we can name instead of using Jira ID.

sfc-gh-pfus · 2025-02-06T06:30:57Z

bindings_test.go

+	interfaceArrayTable := "Snow1313648stageInterface"
+
+	runDBTest(t, func(dbt *DBTest) {
+		dbt.mustExec(fmt.Sprintf("create or replace table %v (c1 integer, c2 string, c3 timestamp_ltz, c4 timestamp_tz, c5 timestamp_ntz, c6 date, c7 time, c8 binary)", arrayInsertTable))


Also boolean and double?

sfc-gh-pfus · 2025-02-06T06:33:29Z

bindings_test.go

+		someTime := time.Date(1, time.January, 1, 12, 34, 56, 123456789, time.UTC)
+		someDate := time.Date(2024, time.March, 18, 0, 0, 0, 0, time.UTC)
+		testingDate := time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC)
+		someBinary := []byte{0x01, 0x02, 0x03}


You can use just 'a', 'b', but your version is also fine :)

sfc-gh-pfus · 2025-02-06T06:35:01Z

bindings_test.go

+			binArr[i] = someBinary
+		}
+		dbt.mustExec(fmt.Sprintf("insert into %v values (?, ?, ?, ?, ?, ?, ?, ?)", arrayInsertTable), Array(&intArr), Array(&strArr), Array(&ltzArr, TimestampLTZType), Array(&tzArr, TimestampTZType), Array(&ntzArr, TimestampNTZType), Array(&dateArr, DateType), Array(&timeArr, TimeType), Array(&binArr))
+		dbt.mustExec("ALTER SESSION SET CLIENT_STAGE_ARRAY_BINDING_THRESHOLD = 1")


Does it work? I thought this was an internal parameter.

sfc-gh-pfus · 2025-02-06T06:41:41Z

bindings_test.go

+				t.Fatal(err)
+			}
+
+			assertEqualE(t, i, bi)


If all values are empty/zeros this comparison succeeds. Maybe it would be better to compare with given values?

sfc-gh-pfus · 2025-02-06T06:42:34Z

bindings_test.go

+	interfaceArrayTable := "Snow1313648stageInterface"
+
+	runDBTest(t, func(dbt *DBTest) {
+		dbt.mustExec(fmt.Sprintf("create or replace table %v (c1 integer, c2 string, c3 timestamp_ltz, c4 timestamp_tz, c5 timestamp_ntz, c6 date, c7 time, c8 binary)", arrayInsertTable))


It is a good practice (I know that missing in many tests) to defer dropping the table.

sfc-gh-pfus · 2025-02-06T06:43:57Z

bindings_test.go

+		testingDate := time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC)
+		someBinary := []byte{0x01, 0x02, 0x03}
+
+		numRows := 5


I'm wondering if such a small number is good enough. There may be a lot of problems with greater number (like 100k) that do not exist with 5 rows. Let's try to increase it and decide if it is not too slow.

sfc-gh-pfus · 2025-02-06T06:47:01Z

converter.go

+
+func convertTimeToTimeStamp(x time.Time, t snowflakeType) string {
+	unixTime, _ := new(big.Int).SetString(fmt.Sprintf("%d", x.Unix()), 10)
+	m, _ := new(big.Int).SetString(strconv.FormatInt(1e9, 10), 10)


Maybe we should propagate the errors here?

sfc-gh-pfus · 2025-02-06T06:49:32Z

converter.go

+	tmNanos, _ := new(big.Int).SetString(fmt.Sprintf("%d", x.Nanosecond()), 10)
+	if t == timestampTzType {
+		_, offset := x.Zone()
+		return fmt.Sprintf("%v %v", unixTime.Add(unixTime, tmNanos), offset/60+1440)


Previously it operated on original time (x in this context), is it intentional to use unixTime here? Your change seems rational to me, but wanted to double check.

sfc-gh-ext-simba-jy and others added 7 commits January 30, 2025 20:03

fix the time shifiting issue and created the testing

8eddf7a

change the table name

391f6d8

Merge branch 'master' into SNOW-1313648

d2484a3

testing

445410a

testing

cf2508a

testing

d0c8e95

fix

8f8b1a4

sfc-gh-ext-simba-jy and others added 6 commits February 4, 2025 17:56

refactoring

c345c77

testing

6ecfcca

Merge branch 'master' into SNOW-1313648

28cc4ea

add more testing cases

3c9024e

Merge branch 'SNOW-1313648' of https://github.com/snowflakedb/gosnowf…

d68f7b8

…lake into SNOW-1313648

remove error

a395bd9

sfc-gh-ext-simba-jy marked this pull request as ready for review February 5, 2025 22:31

sfc-gh-ext-simba-jy requested a review from a team as a code owner February 5, 2025 22:31

sfc-gh-ext-simba-jy changed the title ~~SNOW-1313648~~ SNOW-1313648 GO - Verify value bindings for all field types while exceeding CLIENT_STAGE_ARRAY_BINDING_THRESHOLD Feb 6, 2025

sfc-gh-ext-simba-jy requested review from sfc-gh-pfus, sfc-gh-pmotacki and sfc-gh-pbulawa February 6, 2025 00:40

sfc-gh-pfus reviewed Feb 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-1313648 GO - Verify value bindings for all field types while exceeding CLIENT_STAGE_ARRAY_BINDING_THRESHOLD #1297

SNOW-1313648 GO - Verify value bindings for all field types while exceeding CLIENT_STAGE_ARRAY_BINDING_THRESHOLD #1297

sfc-gh-ext-simba-jy commented Jan 31, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025

sfc-gh-pfus Feb 6, 2025

sfc-gh-pfus Feb 6, 2025

sfc-gh-pfus Feb 6, 2025

sfc-gh-pfus Feb 6, 2025

sfc-gh-pfus Feb 6, 2025

sfc-gh-pfus Feb 6, 2025

sfc-gh-pfus Feb 6, 2025

sfc-gh-pfus Feb 6, 2025

sfc-gh-pfus Feb 6, 2025

sfc-gh-pfus Feb 6, 2025

SNOW-1313648 GO - Verify value bindings for all field types while exceeding CLIENT_STAGE_ARRAY_BINDING_THRESHOLD #1297

Are you sure you want to change the base?

SNOW-1313648 GO - Verify value bindings for all field types while exceeding CLIENT_STAGE_ARRAY_BINDING_THRESHOLD #1297

Conversation

sfc-gh-ext-simba-jy commented Jan 31, 2025 • edited Loading

Description

Checklist

codecov bot commented Feb 5, 2025

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-ext-simba-jy commented Jan 31, 2025 •

edited

Loading