feat(Sink): Enhanced integration with StarRocks #14363

xxhZs · 2024-01-04T08:57:35Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Add starrocks sink partial_update with option
And suport this issue #14822

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)
A new option:

  - name: starrocks.partial_update
    field_type: String
    required: false

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.

fix

neverchanje · 2024-01-30T10:52:38Z

I wonder if we are using the same API for StarRocks and Doris? It seems we tried to differentiate them but nothing is really different.

neverchanje · 2024-01-30T10:54:59Z

BTW, Would you mind sharing the behavior of 'partial_update'? I am not sure if it's an option that should be documented

xxhZs · 2024-01-30T11:00:50Z

I wonder if we are using the same API for StarRocks and Doris? It seems we tried to differentiate them but nothing is really different.

Our Doris and Starlocks use the same HTTP API, but the related configurations are different.

xxhZs · 2024-01-30T11:05:31Z

BTW, Would you mind sharing the behavior of 'partial_update'? I am not sure if it's an option that should be documented

https://docs.starrocks.io/docs/loading/Load_to_Primary_Key_tables/#partial-updates

It seems that only Starrocks supports it

neverchanje

LGTM But I am probably not appropriate for approving. Please ensure another code owner approves as well.

wenym1

Generally LGTM. If this PR is urgent, we can have it merged first, but later we should add some integration tests in CI.

wenym1 · 2024-02-04T07:49:46Z

src/connector/src/sink/starrocks.rs

-            risingwave_common::types::DataType::Jsonb => Err(SinkError::Starrocks(
-                "starrocks can not support import json".to_string(),
-            )),
+            risingwave_common::types::DataType::Jsonb => Ok(starrocks_data_type.contains("json")),


What is the starrocks's type name for json?

wenym1 · 2024-02-04T07:51:11Z

src/connector/src/sink/starrocks.rs

@@ -125,8 +127,8 @@ impl StarrocksSink {
        starrocks_columns_desc: HashMap<String, String>,
    ) -> Result<()> {
        let rw_fields_name = self.schema.fields();
-        if rw_fields_name.len().ne(&starrocks_columns_desc.len()) {
-            return Err(SinkError::Starrocks("The length of the RisingWave column must be equal to the length of the starrocks column".to_string()));
+        if rw_fields_name.len().gt(&starrocks_columns_desc.len()) {


Use > instead of gt.

If we do partial update, should we ensure that the starrrocks pk columns are all included in the sink columns?

Now it is time to calibrate whether the PK of rw and sr are completely consistent, and there is a tendency not to make any modifications. If it is changed to SR and rw not being completely consistent, it may lead to accuracy issues with the update. Our other sinks also follow a similar logic

Do you mean we should consider the case that the downstream pk of SR is changed after the sink is created? I don't think this is what we should consider, because we are not expected to adapt to downstream pk change yet, and users should be required not to change the pk.

Our other sinks also follow a similar logic

This is not ture. For downstreams that really have pk (e.g. jdbc sink), we do check whether the pks specified in the with options match the downstream pk.

I'm just saying that we have code to check PK, but our verification is an exact match, not a superset like other columns（rw.columns.len ＜＝ sr.columns.len）

neverchanje · 2024-02-07T08:19:38Z

src/connector/src/sink/starrocks.rs

@@ -178,7 +180,7 @@ impl StarrocksSink {
                Ok(starrocks_data_type.contains("datetime"))
            }
            risingwave_common::types::DataType::Timestamptz => Err(SinkError::Starrocks(
-                "starrocks can not support Timestamptz".to_string(),
+                "TIMESTAMP WITH TIMEZONE is not supported for Starrocks sink as Starrocks doesn't store time values with timezone information. Please convert to TIMESTAMP first.".to_string(),
            )),
            risingwave_common::types::DataType::Interval => Err(SinkError::Starrocks(


Please refine the error for INTERVAL and BYTEA as well.

fxi fi

Co-authored-by: Xinhao Xu <[email protected]>

github-actions bot added the type/feature label Jan 4, 2024

xxhZs requested a review from hzxa21 January 4, 2024 09:14

add starrocks with option

1918744

fix

xxhZs force-pushed the xxh/add-starrocks-set branch from 3c3d779 to 1918744 Compare January 5, 2024 08:32

xxhZs added the user-facing-changes Contains changes that are visible to users label Jan 11, 2024

xxhZs added ci/run-e2e-clickhouse-sink-tests ci/run- e2e-deltalake-sink-rust-tests and removed ci/run- e2e-deltalake-sink-rust-tests labels Jan 19, 2024

xxhZs and others added 2 commits January 19, 2024 13:09

Merge branch 'main' into xxh/add-starrocks-set

68634e3

Merge branch 'main' into xxh/add-starrocks-set

e620051

xxhZs removed the ci/run-e2e-clickhouse-sink-tests label Jan 30, 2024

add json

4afb4c4

fix

xxhZs force-pushed the xxh/add-starrocks-set branch from e083e86 to 4afb4c4 Compare January 30, 2024 03:17

xxhZs requested review from wenym1 and neverchanje January 30, 2024 03:18

xxhZs changed the title ~~feat(Sink): Add starrocks partial_update option~~ feat(Sink): Enhanced integration with StarRocks Jan 30, 2024

neverchanje approved these changes Jan 31, 2024

View reviewed changes

wenym1 approved these changes Feb 4, 2024

View reviewed changes

xxhZs enabled auto-merge February 7, 2024 03:07

neverchanje reviewed Feb 7, 2024

View reviewed changes

fix

84544b9

fxi fi

xxhZs force-pushed the xxh/add-starrocks-set branch from 986cc5e to 84544b9 Compare February 7, 2024 08:19

xxhZs added this pull request to the merge queue Feb 7, 2024

Merged via the queue into main with commit d50cc77 Feb 7, 2024
27 of 28 checks passed

xxhZs deleted the xxh/add-starrocks-set branch February 7, 2024 09:14

st1page mentioned this pull request Feb 8, 2024

2024-02-07 nexmark performance degradation #15054

Closed

xxhZs added the need-cherry-pick-release-1.7 label Mar 8, 2024

github-actions bot pushed a commit that referenced this pull request Mar 8, 2024

feat(Sink): Enhanced integration with StarRocks (#14363)

64018c7

github-actions bot mentioned this pull request Mar 8, 2024

feat(Sink): Enhanced integration with StarRocks (#14363) #15555

Merged

xxhZs added a commit that referenced this pull request Mar 8, 2024

feat(Sink): Enhanced integration with StarRocks (#14363) (#15555)

423b0d9

Co-authored-by: Xinhao Xu <[email protected]>

BugenZhao mentioned this pull request May 7, 2024

support partial update for Cassandra sink #16607

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Sink): Enhanced integration with StarRocks #14363

feat(Sink): Enhanced integration with StarRocks #14363

xxhZs commented Jan 4, 2024 •

edited by neverchanje

Loading

neverchanje commented Jan 30, 2024

neverchanje commented Jan 30, 2024

xxhZs commented Jan 30, 2024

xxhZs commented Jan 30, 2024 •

edited

Loading

neverchanje left a comment •

edited

Loading

wenym1 left a comment

wenym1 Feb 4, 2024

neverchanje Feb 4, 2024

wenym1 Feb 4, 2024

xxhZs Feb 4, 2024

wenym1 Feb 4, 2024

xxhZs Feb 4, 2024

neverchanje Feb 7, 2024 •

edited

Loading

feat(Sink): Enhanced integration with StarRocks #14363

feat(Sink): Enhanced integration with StarRocks #14363

Conversation

xxhZs commented Jan 4, 2024 • edited by neverchanje Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

neverchanje commented Jan 30, 2024

neverchanje commented Jan 30, 2024

xxhZs commented Jan 30, 2024

xxhZs commented Jan 30, 2024 • edited Loading

neverchanje left a comment • edited Loading

Choose a reason for hiding this comment

wenym1 left a comment

Choose a reason for hiding this comment

wenym1 Feb 4, 2024

Choose a reason for hiding this comment

neverchanje Feb 4, 2024

Choose a reason for hiding this comment

wenym1 Feb 4, 2024

Choose a reason for hiding this comment

xxhZs Feb 4, 2024

Choose a reason for hiding this comment

wenym1 Feb 4, 2024

Choose a reason for hiding this comment

xxhZs Feb 4, 2024

Choose a reason for hiding this comment

neverchanje Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

xxhZs commented Jan 4, 2024 •

edited by neverchanje

Loading

xxhZs commented Jan 30, 2024 •

edited

Loading

neverchanje left a comment •

edited

Loading

neverchanje Feb 7, 2024 •

edited

Loading