fix(cdc): fix enum to varchar in postgres-cdc #16423
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
KeXiangWang
force-pushed
the
wkx/enum-in-cdc
branch
5 times, most recently
from
f43d3c9
to
247db90
Compare
KeXiangWang
force-pushed
the
wkx/enum-in-cdc
branch
from
ef54d26
to
c94f74c
Compare
src/connector/src/parser/postgres.rs
Outdated
|
|
||
| #[ignore] | ||
| #[test] | ||
| fn enum_parser_integration_test() { |
There was a problem hiding this comment.
Is it possible to do some unit test instead of integration test here? I guess e2e test has already played the integration role? Should have automated test here.
There was a problem hiding this comment.
In this test, I want to make sure two things:
EnumParsercorrectly understood the data queried from pg.- The data transformed from
EnumParsercan be correctly understood by pg.
So the test cannot run without pg. Here, as I have marked as #[ignore], I just want to provide a simple tool to test EnumParser. e2e test can already verify its correctness.
KeXiangWang
force-pushed
the
wkx/enum-in-cdc
branch
from
92c139d
to
9c8fb2d
Compare
| _ty: &Type, | ||
| raw: &'a [u8], | ||
| ) -> Result<Self, Box<dyn std::error::Error + 'static + Sync + Send>> { | ||
| Ok(EnumString(String::from_utf8_lossy(raw).into_owned())) |
There was a problem hiding this comment.
I realized that our native postgres parser cannot fallback upstream data types that we don't support to varchar.
Besides Enum, for example if user want to map INET to varchar, we cannot make it.
Is it possible to apply this implementation, blindly store as a string, to other postgres data types? @KeXiangWang
There was a problem hiding this comment.
It depends on whether the type is serialized directly as utf8 string. We need to test different types to verify.
There was a problem hiding this comment.
#16455
Just created an issue to track
There was a problem hiding this comment.
Another choice is that, when backfilling, instead of querying PG with SELECT {} FROM {} here and converting in RW, we can directly convert the data to varchar in pg.
For example, if we find an enum column E is to be converted to varchar, we can directly run SELECT E::varchar FROM, so that we don't need to handle it in our codes.
| if let Kind::Array(item_type) = row.columns()[i].type_().kind() | ||
| && let Kind::Enum(_) = item_type.kind() | ||
| { | ||
| let res = row.try_get::<_, Option<Vec<EnumString>>>(i); |
There was a problem hiding this comment.
Maybe affect other cases as well: have we tested null inside an array? Maybe we need Option<Vec<Option<T>>>
There was a problem hiding this comment.
Just tested. Yes, we need Option<Vec<Option<T>>>. I'll create a new PR to fix.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
fix #16392
Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.