refactor: refine error message and test for FORMAT UPSERT #17397

xxchan · 2024-06-21T10:02:34Z

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

What's changed and what's your intention?

Motivation: previously the error message is very silly:

It first errors with INCLUDE KEY clause must be set
After it's added, it errors with Primary key must be specified

Why can't we hint enough information in one shot?

For CREATE SOURCE UPSERT, it's even more silly: It will go through the same error above, then 3. tell you Source does not support PRIMARY KEY constraint.

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)

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.

xxchan · 2024-06-21T10:02:48Z

refactor: refine error message and test for FORMAT UPSERT #17397 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @xxchan and the rest of your teammates on Graphite

xxchan · 2024-06-21T10:09:46Z

e2e_test/source_inline/kafka/include_key_as.slt

@@ -0,0 +1,284 @@
+control substitution on


The changes to this test can be reviewed commit by commit

what does this command mean?

Replace ${} to env vars in the slt.

To use ${RISEDEV_KAFKA_WITH_OPTIONS_COMMON}, this needs to be opened.

tabVersion

LGTM, thanks for refining the error msg

tabVersion · 2024-06-24T04:06:36Z

e2e_test/source_inline/kafka/include_key_as.slt

@@ -0,0 +1,284 @@
+control substitution on


what does this command mean?

xiangjinwu · 2024-06-24T04:16:25Z

src/frontend/src/handler/create_source.rs

+            _ => {
+                // TODO: enhance error message for other formats


Debezium (DebeziumMongo), Canal, Maxwell all require table rather than source.

Unsure about none / native.

Yeah, I thought only PLAIN is for SOURCE. Now I'm not sure when I found none/native. 🤪

native is for datagen, so it should work in both source and table. Not sure about None 😇

According to the commend, native is for schema change

Link to the original PR: #12306

TL;DR: Keywords like NATIVE will never be explicitly written by users in their SQL, but appear after normalization and persistence. When we're going to do schema change, we have to reparse the SQL (under current implementation) so NATIVE must be accepted as a keyword.

Signed-off-by: xxchan <[email protected]>

src/frontend/src/handler/create_source.rs

Signed-off-by: xxchan <[email protected]>

BugenZhao

L🥰TM. The tests for error reporting is quite comprehensive.

e2e_test/source_inline/kafka/include_key_as.slt

src/frontend/src/handler/create_source.rs

BugenZhao · 2024-06-24T05:15:02Z

src/frontend/src/handler/create_source.rs

+            _ => {
+                // TODO: enhance error message for other formats


Link to the original PR: #12306

TL;DR: Keywords like NATIVE will never be explicitly written by users in their SQL, but appear after normalization and persistence. When we're going to do schema change, we have to reparse the SQL (under current implementation) so NATIVE must be accepted as a keyword.

BugenZhao · 2024-06-24T05:16:08Z

src/frontend/src/handler/create_source.rs

@@ -750,6 +750,18 @@ pub(crate) fn bind_all_columns(
    }
 }

+fn hint_upsert(encode: &Encode) -> String {
+    format!(
+        r#"Hint: For FORMAT UPSERT ENCODE {encode:}, INCLUDE KEY must be specified and the key column must be used as primary key.


How I wish thiserror provides something similar to this:

https://docs.rs/snafu/latest/snafu/derive.Snafu.html#providing-data-beyond-the-error-trait

Co-authored-by: Bugen Zhao <[email protected]>

xiangjinwu · 2024-06-24T04:37:39Z

src/frontend/src/handler/create_source.rs

-                        "Primary key must be specified to {} when creating source with FORMAT UPSERT ENCODE {:?}",
-                        key_column_name.unwrap(), encode))))
+                        "Primary key must be specified to {}\n\n{}",
+                        include_key_column_name.unwrap(),


nit: if let Some to avoid this unwrap.

Signed-off-by: xxchan <[email protected]>

stdrc

👏👏

xxchan marked this pull request as ready for review June 21, 2024 10:02

xxchan requested a review from a team as a code owner June 21, 2024 10:02

github-actions bot added the type/refactor label Jun 21, 2024

xxchan force-pushed the include-test branch from 828b5fa to b6daaac Compare June 21, 2024 10:04

xxchan changed the title ~~refactor: move json schema to codec crate~~ refactor: refine error message and test for FORMAT UPSERT Jun 21, 2024

xxchan force-pushed the include-test branch 2 times, most recently from a617164 to 3562def Compare June 21, 2024 10:09

xxchan commented Jun 21, 2024

View reviewed changes

xxchan requested review from tabVersion, xiangjinwu, BugenZhao and st1page June 21, 2024 10:24

tabVersion approved these changes Jun 24, 2024

View reviewed changes

e2e_test/source_inline/kafka/include_key_as.slt

@@ -0,0 +1,284 @@

control substitution on

Copy link

Contributor

tabVersion Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this command mean?

xiangjinwu reviewed Jun 24, 2024

View reviewed changes

xxchan added 7 commits June 24, 2024 12:25

move test

6ec29a9

Signed-off-by: xxchan <[email protected]>

modernize test

0b37f4b

Signed-off-by: xxchan <[email protected]>

add more error test

381c384

Signed-off-by: xxchan <[email protected]>

enhance error message for FORMAT UPSERT

e6f37af

Signed-off-by: xxchan <[email protected]>

refine test

0fbfcd7

Signed-off-by: xxchan <[email protected]>

fix test

111dba6

Signed-off-by: xxchan <[email protected]>

minor

bb39fd0

Signed-off-by: xxchan <[email protected]>

xxchan force-pushed the include-test branch from 8226773 to bb39fd0 Compare June 24, 2024 04:26

xiangjinwu reviewed Jun 24, 2024

View reviewed changes

src/frontend/src/handler/create_source.rs Outdated Show resolved Hide resolved

fix

2e02b73

Signed-off-by: xxchan <[email protected]>

BugenZhao approved these changes Jun 24, 2024

View reviewed changes

Update src/frontend/src/handler/create_source.rs

641dbb9

Co-authored-by: Bugen Zhao <[email protected]>

xxchan enabled auto-merge June 24, 2024 05:37

xiangjinwu approved these changes Jun 24, 2024

View reviewed changes

nit

d04bed5

Signed-off-by: xxchan <[email protected]>

xxchan added this pull request to the merge queue Jun 24, 2024

st1page approved these changes Jun 24, 2024

View reviewed changes

stdrc approved these changes Jun 24, 2024

View reviewed changes

Merged via the queue into main with commit 4f8bfc4 Jun 24, 2024
31 of 32 checks passed

xxchan deleted the include-test branch June 24, 2024 06:56

xiangjinwu mentioned this pull request Jul 15, 2024

feat(source): Avro with AWS Glue Schema Registry (#17605) #17677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: refine error message and test for FORMAT UPSERT #17397

refactor: refine error message and test for FORMAT UPSERT #17397

xxchan commented Jun 21, 2024 •

edited

Loading

xxchan commented Jun 21, 2024 •

edited

Loading

xxchan Jun 21, 2024

tabVersion Jun 24, 2024

xxchan Jun 24, 2024

tabVersion left a comment

tabVersion Jun 24, 2024

xiangjinwu Jun 24, 2024 •

edited

Loading

xxchan Jun 24, 2024

tabVersion Jun 24, 2024

xxchan Jun 24, 2024

BugenZhao Jun 24, 2024

BugenZhao left a comment

BugenZhao Jun 24, 2024

BugenZhao Jun 24, 2024

xiangjinwu Jun 24, 2024

stdrc left a comment

refactor: refine error message and test for FORMAT UPSERT #17397

refactor: refine error message and test for FORMAT UPSERT #17397

Conversation

xxchan commented Jun 21, 2024 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

xxchan commented Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tabVersion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangjinwu Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugenZhao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stdrc left a comment

Choose a reason for hiding this comment

xxchan commented Jun 21, 2024 •

edited

Loading

xxchan commented Jun 21, 2024 •

edited

Loading

xiangjinwu Jun 24, 2024 •

edited

Loading