-
Notifications
You must be signed in to change notification settings - Fork 592
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
connector: support protobuf map type in source #13387
Comments
This comment was marked as resolved.
This comment was marked as resolved.
https://protobuf.dev/programming-guides/proto3/#maps
I will focus on the downside of each approach:
|
Thanks for your detailed exploration! Personally, I would stand for
|
This comment was marked as resolved.
This comment was marked as resolved.
-1 for
Here, I prefer |
It's okay to make it |
It's inevitable since we have already supported |
Thinking from the users' perspective, if we do really use
Adding up together, I would rather introduce a new |
Putting development time aside, a native As for
This is not a big deal. Proto3 has an official definition of how proto maps to JSON. But do note that int64 maps to string. |
Yeah, I understand this, and I guess it's also the major concern of all of us. For me, because |
Btw do we use the official proto3 json mapping in |
This comment was marked as resolved.
This comment was marked as resolved.
I guess @xiangjinwu means an |
My concernings for a native
As above, it might not be that simple... |
Had another offline discussion with @fuyufjh and we aligned on these:
|
So we have a new usage now #14215 😄 |
I just realized that many systems introduced dedicated
Perhaps Postgres is the special one... Shall we add support for |
Concern of using
It is also easy to convert from these forms into a
My thoughts:
|
To elaborate this point a bit: it's actually a big deal, and another big drawback of JSON -- It lacks native support for a lot of types, and thus leaving there many choices of encoding to JSON and decoding from JSON, which means we need to provide a lot of config options. The encoding problem include:
Different cases include:
My opinion for the last question is that we don't provide encoding option for this case, and just choose our opinionated one. Oh, it seems xiangjin already talked about this:
|
This issue has been open for 60 days with no activity. If you think it is still relevant today, and needs to be done in the near future, you can comment to update the status, or just manually remove the You can also confidently close this issue as not planned to keep our backlog clean. |
Prior to #12126, a
map
field is accepted and map tostruct<key, value>[]
in RisingWave, which might not be expected as suggested by @neverchanje so disabled there.JSONB
might be another good candidate, which supports efficient indexing.The text was updated successfully, but these errors were encountered: