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

[SNOW-1669514] Honor the Valuer/Stringer methods to resolve #1209 #1211

Merged

Conversation

ChronosMasterOfAllTime
Copy link
Contributor

@ChronosMasterOfAllTime ChronosMasterOfAllTime commented Sep 19, 2024

Description

SNOW-1669514 - Solve the issue with UUID getting double quoted due to new array function #1209

This adds the ability to check the valuer/stringer interfaces exists

Checklist

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

Proof test fails.

export SKIP_SETUP=true && go test -run ^TestValueToString$ github.com/snowflakedb/gosnowflake
--- FAIL: TestValueToString (0.00s)
    --- FAIL: TestValueToString/UUID_-_should_return_string (0.00s)
        assert_test.go:103: expected "json" to be empty, but was not. . Thrown from gosnowflake/converter_test.go:290 +0xd4
            testing.tRunner(0xc0000bf520, 0x100e3a478)
        assert_test.go:103: expected ""1414b0f8-e56a-4e14-995b-774d87ea9d73"" to be equal to "1414b0f8-e56a-4e14-995b-774d87ea9d73" but was not. . Thrown from gosnowflake/converter_test.go:291 +0x197
            testing.tRunner(0xc0000bf520, 0x100e3a478)

@ChronosMasterOfAllTime ChronosMasterOfAllTime requested a review from a team as a code owner September 19, 2024 20:50
@ChronosMasterOfAllTime ChronosMasterOfAllTime changed the title Try to recreate the issue with UUID to solve #1209 WIP: Try to recreate the issue with UUID to solve #1209 Sep 19, 2024
@ChronosMasterOfAllTime ChronosMasterOfAllTime changed the title WIP: Try to recreate the issue with UUID to solve #1209 Recreate the issue with UUID to solve #1209 Sep 19, 2024
converter.go Outdated Show resolved Hide resolved
@ChronosMasterOfAllTime ChronosMasterOfAllTime changed the title Recreate the issue with UUID to solve #1209 Honor the Valuer/Stringer methods to resolve #1209 Sep 20, 2024
@ChronosMasterOfAllTime ChronosMasterOfAllTime changed the title Honor the Valuer/Stringer methods to resolve #1209 [SNOW-1669514] Honor the Valuer/Stringer methods to resolve #1209 Sep 23, 2024
Copy link
Collaborator

@sfc-gh-pfus sfc-gh-pfus left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Can you take a look at my comments?

converter.go Show resolved Hide resolved
converter.go Show resolved Hide resolved
converter.go Show resolved Hide resolved
converter.go Outdated Show resolved Hide resolved
@ChronosMasterOfAllTime
Copy link
Contributor Author

@sfc-gh-pfus added tests to structured R/W and Driver. Will attempt to run the tests

bind_uploader.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
structured_type_read_test.go Outdated Show resolved Hide resolved
@sfc-gh-pfus
Copy link
Collaborator

@ChronosMasterOfAllTime good job! But correct me if I'm wrong, but as far as I can see, there are only tests for embedding uuid in structured types and not as inserts into/selects from a regular column? Can you add such a test case as well (probably somewhere in driver_test.go)?

@ChronosMasterOfAllTime
Copy link
Contributor Author

ChronosMasterOfAllTime commented Nov 20, 2024

@ChronosMasterOfAllTime good job! But correct me if I'm wrong, but as far as I can see, there are only tests for embedding uuid in structured types and not as inserts into/selects from a regular column? Can you add such a test case as well (probably somewhere in driver_test.go)?

There is a test added in driver_test.go

@ChronosMasterOfAllTime
Copy link
Contributor Author

@sfc-gh-pfus need support on this one:
(I was running structured_write_test.go from the master branch first to ensure it worked correctly)

Getting the following error:

--- FAIL: TestBindingObjectWithSchema (0.67s)
    driver_test.go:276: error on exec context [[query too large to print]]: 002040 (42601): SQL compilation error:
        Unsupported data type 'OBJECT(s VARCHAR(16777216), b NUMBER(38,0), i16 NUMBER(38,0), i32 NUMBER(38,0), i64 NUMBER(38,0), f32 FLOAT, f64 FLOAT, nfraction NUMBER(38,9), bo BOOLEAN, bi BINARY(8388608), date DATE, time TIME(9), ltz TIMESTAMP_LTZ(9), ntz TIMESTAMP_NTZ(9), tz TIMESTAMP_TZ(9), so OBJECT(s VARCHAR(16777216), i NUMBER(38,0)), sArr ARRAY(VARCHAR(16777216)), f64Arr ARRAY(FLOAT), someMap MAP(VARCHAR(16777216), BOOLEAN))'.

bind_uploader.go Outdated Show resolved Hide resolved
driver_test.go Show resolved Hide resolved
driver_test.go Outdated Show resolved Hide resolved
driver_test.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@sfc-gh-pfus
Copy link
Collaborator

@sfc-gh-pfus need support on this one: (I was running structured_write_test.go from the master branch first to ensure it worked correctly)

Getting the following error:

--- FAIL: TestBindingObjectWithSchema (0.67s)
    driver_test.go:276: error on exec context [[query too large to print]]: 002040 (42601): SQL compilation error:
        Unsupported data type 'OBJECT(s VARCHAR(16777216), b NUMBER(38,0), i16 NUMBER(38,0), i32 NUMBER(38,0), i64 NUMBER(38,0), f32 FLOAT, f64 FLOAT, nfraction NUMBER(38,9), bo BOOLEAN, bi BINARY(8388608), date DATE, time TIME(9), ltz TIMESTAMP_LTZ(9), ntz TIMESTAMP_NTZ(9), tz TIMESTAMP_TZ(9), so OBJECT(s VARCHAR(16777216), i NUMBER(38,0)), sArr ARRAY(VARCHAR(16777216)), f64Arr ARRAY(FLOAT), someMap MAP(VARCHAR(16777216), BOOLEAN))'.

I tried it locally and it worked. I started build on Github Actions, let's check it. By looking at the message - I believe it because you didn't add this UUID to object definition in table, but in the current version it is present, so let's hope it works.

@ChronosMasterOfAllTime
Copy link
Contributor Author

What I mean is I tried this on a clean branch and got the same error. Is it something to do with my snowflake instance? (I finally got the ability to test against our QA env)

@sfc-gh-pfus
Copy link
Collaborator

So probably yes, probably you don't have access to some features. But it works on CI, so it is good enough.

Copy link
Collaborator

@sfc-gh-pfus sfc-gh-pfus left a comment

Choose a reason for hiding this comment

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

One last comment and we are good to go!

bindings_test.go Outdated Show resolved Hide resolved
@ChronosMasterOfAllTime
Copy link
Contributor Author

@sfc-gh-pfus doh! linter errors fixed

@ChronosMasterOfAllTime
Copy link
Contributor Author

@sfc-gh-dheyman I ran the linter locally and noticed the fmt job had some whitespace changes; added in this commit b07b709

@sfc-gh-pfus sfc-gh-pfus merged commit d91e009 into snowflakedb:master Dec 10, 2024
35 of 38 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2024
@sfc-gh-pfus
Copy link
Collaborator

Part of the journey is the end. @ChronosMasterOfAllTime, thank you very much for all the work you put into this PR and for your huge amount of patience!

@ChronosMasterOfAllTime ChronosMasterOfAllTime deleted the SNOW-1669514_issue_1209 branch December 10, 2024 16:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants