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: allow inserting Map using singlequoted JSON #123

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

Conversation

mtrimolet
Copy link

Inserting a value for a Map field can be done using a key-value format which is a kind of singlequoted JSON (see Map(key, value) | ClickHouse Docs.

This is a proposal to use this feature from CH, enabling insertion for Map fields.

@TimonKK
Copy link
Owner

TimonKK commented Aug 15, 2022

Able to you add some test for null insert?

@mtrimolet
Copy link
Author

It seems the Map type can't be a Nullable by design (error below).

Query : CREATE TABLE ... rec3 Nullable(Map(String, UInt8)) [cut for clarity]
Result : Error: Nested type Map(String, UInt8) cannot be inside Nullable type. (ILLEGAL_TYPE_OF_ARGUMENT) (version 21.12.4.1 (official build))

As for the other way around, yes I can add this in the tests if you confirm that it's what you intended.

Query : CREATE TABLE ... rec3 Map(String, Nullable(UInt8)) [cut for clarity]
Row Data (JS) : { ... rec3: { a: null } ... } [cut for clarity]
Result : OK

@mtrimolet
Copy link
Author

There's a point to be made about nested Map/Array (see citation below), for which I expect this implementation to fail.
But maybe it should be issued separately as it concerns more general aspects of this lib.

From Map(key, value) | ClickHouse Docs :

Parameters

  • key — [...]
  • value — The value part of the pair. Arbitrary type, including Map and Array.

@Sh4yy
Copy link

Sh4yy commented Aug 30, 2022

Any updates on this? Is there a solution that I can use for now?

@mtrimolet
Copy link
Author

There's a point to be made about nested Map/Array (see citation below), for which I expect this implementation to fail.

Actually it seems to work just fine.

@Sh4yy
Copy link

Sh4yy commented Sep 9, 2022

Yep, I'm using a fork of your PR at the moment. I would appreciate it if this gets merged so I can switch over.

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