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

Null support for inserts #84

Closed
oerdogmus opened this issue Jul 1, 2023 · 3 comments · Fixed by #93
Closed

Null support for inserts #84

oerdogmus opened this issue Jul 1, 2023 · 3 comments · Fixed by #93

Comments

@oerdogmus
Copy link

Client version: v0.312.0

I have a use case where I insert multiple records with different fields are being nullable. For example, my table schema is as follows:

id: int
name: string
order: int (nullable)
url: string (nullable)

order and url fields are nullable so when I have 2 records:

(1, "first record", nil, "url")
(2, "second record", 1, nil)

I expected to execute the following SQL query (with arguments):

INSERT INTO my_table (id, name, order, url)
VALUES
(1, 'first record', NULL, 'url'),
(2, 'second record', 1, NULL);

However, when ExecContext function is called with driver.NamedValue that has nil as a value, it throws exception in type conversion function here.

To support nil values, we can change the type conversion for nil to:

case nil:
    return "NULL", nil

I am curious what your thoughts are. If this is not a desirable behaviour, is there another workaround for the problem?

@mosabua
Copy link
Member

mosabua commented Jul 4, 2023

I think this is a bug and your suggestion a potential bug fix. Could you maybe send a pull request that fixes this and also test the fix?

Also wdyt @nineinchnick and @losipiuk ?

@losipiuk
Copy link
Member

losipiuk commented Jul 5, 2023

Yeah - it looks like a bug to me. We currently do not have any DML test coverage for go client - so there is a chance that there are other bugs in code, similar to this one.
@oerdogmus if you are up for a PR would you also be able to add some test coverage for DML in trino_test.go?

@nineinchnick does proposed fix look ok to you?

@nineinchnick
Copy link
Member

Yes, it looks good. We definitely need more tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants