-
Notifications
You must be signed in to change notification settings - Fork 576
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
fix(sink): fix starrocks doris and clickhouse decimal #15664
Conversation
Intuitively this might work, but I am not sure if this will be too implicit for users. Did you check how flink connector handle these cases? |
could you add tests for these edge cases in the integration tests? Thanks! |
d8dd79c
to
2ca5821
Compare
4c80360
to
65fb97b
Compare
65fb97b
to
36a5fbb
Compare
Is the change only compatible to the latest version of starrocks? Does it work on some previous versions of starrocks? If not we may add in our document and release note to notify users to be careful with the upgrade. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM.
Can you paste some links from starrocks and doris about the related changes in the PR to explain the reason for removing the decimal config map from starrocks and changing the decimal config map from (u8, u8) to u8 for doris?
test with 2.5.18 -> laster(The versions we support), No problem |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Previously we would report an error when insert ck doris starrocks nan inf -inf, now we have made the change
in clickhouse ,if clickhouse is nullable, will insert null. If not, we will insert 0.
in doris/storrocks, if decimal is out of bounds or inf -inf nan, we insert null.
Due to the starrocks update, we removed some of the calibration logic for decimal
The reason for the change is that
previously we needed precision to make precision corrections, e.g. if we had a 1.123456 and starrocks had a precision of 3, we would need to convert it to 1.123 first, but recent versions of starrocks have been able to convert it automatically, so it doesn't require us to do this (doris is still required)
Regarding the maximum number of bits, as long as the maximum number of bits is exceeded, there is no way for us to insert into the downstream. Previously, we would judge the number of bits because we wanted to report the error ahead of time, but then we found out that there is no point in doing so, so we remove this logic
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
Previously we would report an error when insert ck doris starrocks nan inf -inf, now we have made the change
in clickhouse ,if clickhouse is nullable, will insert null. If not, we will insert 0.