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

Open
wants to merge 42 commits into
base: master
Choose a base branch
from

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 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

ChronosMasterOfAllTime commented Dec 2, 2024

@sfc-gh-pfus I was out last week and taking a look now

EDIT: Just need your assistance with structured types testing given those alter session statements that only run in your ecosystem.

@sfc-gh-pfus
Copy link
Collaborator

Hi, I had an impression these parameters should be enabled by default now for everyone. Can you check if the tests pass without enabling them? In the meantime - I started an initiative to verify these params on our side.

@ChronosMasterOfAllTime
Copy link
Contributor Author

Hi, I had an impression these parameters should be enabled by default now for everyone. Can you check if the tests pass without enabling them? In the meantime - I started an initiative to verify these params on our side.

Tests will not pass because it returns all objects as strings to the driver and the Fields for each row will not be set. Because the session cannot be set it doesn't set the context for it either (verified with delve).

@sfc-gh-pfus
Copy link
Collaborator

Sure, so these params are indeed needed. I'll let you know my findings if these parameters should be publicly enabled or not and what are the next steps.

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.

3 participants