fix: misuse of props and options in `CREATE SOURCE` #13762

Rossil2012 · 2023-12-01T08:39:18Z

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

What's changed and what's your intention?

Properties in WITH clause and options in FORMAT ... ENCODE ... are misused. Options are merged into props, and parameters defined in options will be further looked up within props. This may result in the following consequences:

Duplicate keys in both options and props
Users fill parameters in wrong clause and get no error/warning
Checks of consuming props/options can be corrupted (see fix: temp disable option check on source options #13362 feat(source): deny unknown fields for WITH options #13654)

Here are some keys that supposed to be in the options instead of props:

schema.* (except for schema.name in db_cdc)
message
key.message
without_header
delimiter

Also found a misuse in the kafka doc: https://docs.risingwave.com/docs/current/ingest-from-kafka/#connector-parameters.
schema.registry.username and schema.registry.password should be defined in ENCODE ... FORMAT. cc @neverchanje.

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.

…-misuse

codecov · 2023-12-04T04:49:00Z

Codecov Report

Attention: 193 lines in your changes are missing coverage. Please review.

Comparison is base (38e54c7) 68.05% compared to head (47e6ff3) 67.96%.

Files	Patch %	Lines
src/frontend/src/handler/create_source.rs	52.45%	116 Missing ⚠️
src/connector/src/source/mod.rs	0.00%	21 Missing ⚠️
src/meta/src/manager/catalog/mod.rs	46.87%	17 Missing ⚠️
src/connector/src/parser/mod.rs	33.33%	6 Missing ⚠️
src/connector/src/source/base.rs	57.14%	6 Missing ⚠️
src/source/src/source_desc.rs	54.54%	5 Missing ⚠️
src/connector/src/source/external.rs	33.33%	4 Missing ⚠️
src/meta/service/src/ddl_service.rs	0.00%	3 Missing ⚠️
src/stream/src/from_proto/source/trad_source.rs	0.00%	3 Missing ⚠️
src/batch/src/executor/source.rs	0.00%	2 Missing ⚠️
... and 9 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13762      +/-   ##
==========================================
- Coverage   68.05%   67.96%   -0.10%     
==========================================
  Files        1536     1537       +1     
  Lines      265406   265680     +274     
==========================================
- Hits       180628   180569      -59     
- Misses      84778    85111     +333

Flag	Coverage Δ
rust	`67.96% <57.11%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xxchan · 2023-12-04T09:54:42Z

src/frontend/src/catalog/source_catalog.rs

+        let properties = WithOptions::new(prost.properties.clone()).into_inner();
+        let options = WithOptions::new(prost.options.clone()).into_inner();


We might need to rename the type alias now... 🥵

Besides, properties and options are not meaningful names. Can we change them to sth like with_options and encode_format_options/row_options?

Also in all other places, including proto

How about with_properties and row_options?

the most of fields in option are related to parser behavior, I prefer encode_option or parser_option

I have been calling them format options for sink 😂

parser_options

Wait a minute, the syntax seems to be FORMAT ... ENCODE ... (...), so at least it should be format_encode_options 🤣

From the syntax, the options indeed look like it's for ENCODE 🤔.

From the parser's definition it seems to be called row_options, so I guess this is what it should be like when it was designed (and not only for ENCODE). So might worth asking @st1page what's the best name for it.

risingwave/src/sqlparser/src/ast/statement.rs

Lines 338 to 342 in b5c3f5d

pub struct ConnectorSchema {

pub format: Format,

pub row_encode: Encode,

pub row_options: Vec<SqlOption>,

}

the most of fields in option are related to parser behavior, I prefer encode_option or parser_option

I'm wondering whether parser_options will be confusing to users. We need to consider this in the error messages (maybe also syntax reference, but currently it also doesn't have a name)

c.c. @tabVersion
I think it is the historical reason... format_encode_options LGTM.
Btw, we might need some options in the KEY ENCODE in the future.

format upsert encode avro ( schema.registry = 'http://message_queue:8081/', schema.registry.name.strategy = 'typo') key encode avro ( schema.registry = 'http://message_queue:8081/', schema.registry.name.strategy = 'typo') )

or maybe this syntax

format upsert encode avro ( schema.registry = 'http://message_queue:8081/', schema.registry.name.strategy = 'typo') INCLUDE KEY AS kafka_key ENCODE avro ( schema.registry = 'http://message_queue:8081/', schema.registry.name.strategy = 'typo') )

Considering the unknown part of the other options... I think format_encode_options is the most specific and clear name.

I think it is the historical reason... format_encode_options LGTM.
Btw, we might need some options in the KEY ENCODE in the future.

+1 for format_encode_options
for KEY ENCODE, I think it is ok to add some fields in the options because the clause is optional in SQL and we don't expect it as strong as the value part.

xxchan

Thanks for the work. Can you explain a little about what's changed in the code? Especially:

Whether is the separated options used
Whether and how is backwards compatibility achived

xxchan · 2023-12-04T09:58:07Z

src/frontend/src/handler/create_source.rs

    with_properties: &HashMap<String, String>,
+    row_options: &mut BTreeMap<String, String>,


The difference between the two's type might be confusing (especially mut). Better to add some comments.

We use &mut for row_options because in the bind_columns_from_source function fields of options are consumed (i.e. row_options.remove(&key)). And we will check options.is_empty() in the end.
https://github.com/risingwavelabs/risingwave/blob/main/src/frontend/src/handler/create_source.rs#L572-L584

The difference between the two's type might be confusing (especially mut).

We hope to consume both configs here but it seems impossible for connector props. Let's keep format_encode_options safe from unknown fields.

xxchan · 2023-12-04T12:59:55Z

Besides, how is #12974 related?

Rossil2012 · 2023-12-05T02:24:01Z

Can you explain a little about what's changed in the code?

In create_source and create_table handler, keys are identified to be used in props and options according to docs.
Add options field in PbSource
Add column for options field in sql meta store.

Whether is the separated options used

The options are separated and persisted in meta, but are not further used for now. But as @tabVersion inform that there is a plan to support alter source, the separated options might be useful in the future.

Whether and how is backwards compatibility achived

There is no convenient or absolutely correct way to do this. I just carefully go through the code from frontend to meta and check the docs to check if the key is supposed to be in options other than props. And to ensure options are consumed correctly.

BugenZhao · 2023-12-06T03:30:02Z

src/frontend/src/catalog/source_catalog.rs

+        let properties = WithOptions::new(prost.properties.clone()).into_inner();
+        let options = WithOptions::new(prost.options.clone()).into_inner();


BTW, should we consider backward compatibility here?

For now, legacy codes that try to find keys of format_encode_options in with_properties are identified and corrected. So unmerging format_encode_options from with_properties will not result in key-not-found-like errors. Is this the backward compatibility you suggested?

IMO the most important thing is: already created source can still work. It seems compute node doesn't use format_encode_options at all and thus not broken? (I'm not very sure. Correct me if I'm wrong.)

Besides is the semantics change on the frontend. I'm still not sure whether sth is changed (i.e., can work previously, but can't work now, and vice versa). But actually that's acceptable to me...

Glad to hear others' thoughts about detailed backward compatibility concerns. @BugenZhao zkss

already created source can still work.

Yes!

It seems compute node doesn't use format_encode_options at all and thus not broken?

Also the meta service part: does it handle the cases that the new fields do not exist?

TBH, I'm not that familiar with the connector part. 🤣 Practice is the sole criterion for testing truth.

I just added a backwards-compat test for invalid WITH options #13824. Can we or do we need to add similar stuff for this PR?

IMO the most important thing is: already created source can still work.

Do you mean that, for example, a user creates a source using RW 1.4, and then upgrades to RW 1.5. So codes in RW 1.5 should recognize it as legacy source and try to find option keys in properties. Is this hot-update issue you concerned?

Got it. We do need to add more codes to handle this issue.

FYI 😇 this is my solution for rejecting unknown WITH
2e39b51#diff-a80dd5cfb3d2b493a15182ad5c447741b10820fa16b8eea2719b9c0ae6f003a4R415-R418

tabVersion · 2023-12-10T09:08:51Z

fix the doc mismatch: https://github.com/risingwavelabs/risingwave-docs/pull/1627

src/meta/src/manager/catalog/database.rs

tabVersion

generally LGTM, good work!

proto/catalog.proto

src/connector/src/source/mod.rs

fuyufjh · 2023-12-12T08:03:05Z

src/frontend/src/handler/create_source.rs

+    )))
+}
+
+fn consume_aws_config_from_options(format_encode_options: &mut BTreeMap<String, String>) {


Suggested change

fn consume_aws_config_from_options(format_encode_options: &mut BTreeMap<String, String>) {

fn remove_aws_config_from_options(format_encode_options: &mut BTreeMap<String, String>) {

dashboard/pages/api/streaming.ts

fuyufjh

The rest LGTM

Rossil2012 · 2023-12-13T11:16:26Z

src/connector/src/source/mod.rs

+    PREFIXES.iter().any(|prefix| key.starts_with(prefix))
+        || (key == "endpoint" && !connector.eq_ignore_ascii_case(KINESIS_CONNECTOR))


endpoint is also found used (and only used) in Kinesis properties, so we move endpoint to format_encode_options if the connector is not Kinesis.

pub struct KinesisCommon { #[serde(rename = "stream", alias = "kinesis.stream.name")] pub stream_name: String, #[serde(rename = "aws.region", alias = "kinesis.stream.region")] pub stream_region: String, #[serde(rename = "endpoint", alias = "kinesis.endpoint")] pub endpoint: Option<String>,

I have no experience on aws config names used for connecting to kinesis (endpoint / kinesis.endpoint) vs names used for reading schema definition from s3 (endpoint_url). Would rely on @tabVersion with more experience to make sure the names are listed correctly, and with proper tests.

My previous comment used the word copy instead of move. I am not sure if some names in legacy with_properties are actually used by both with and format encode. Moving them away from with_properties may break things. Again, whether this is an real issue depends on the meaning / usage of specific names, which has been obscure for a long time every time we talk about with properties. Just bringing the idea up and would rely on your judgement.

Studied serde rename and alias, and it turns out the only name shared by both is endpoint, which would not be moved in the case of kinesis. And existing kinesis connector are not using format encode from s3. (or we need to copy endpoint even when it is kinesis)

All other names for reading format encode from s3 has already been listed above properly.

Just for clarity, this is a concrete example of the current situation:

create [source | table] t with ( connector = 'kinesis', stream = 't', aws.region = 'r0', endpoint = 'e0', aws.credentials.access_key_id = 'ak0', aws.credentials.secret_access_key = 'sk0' ) format plain encode avro ( schema.location = 's3://foo/bar', region = 'r1', access_key = 'ak1', secret_key = 'sk1' );

It is unfortunate we seem to have no test or doc example about reading schema definition from s3.

xiangjinwu

Rest LGTM.

xiangjinwu · 2023-12-14T08:25:12Z

src/connector/src/source/mod.rs

+    PREFIXES.iter().any(|prefix| key.starts_with(prefix))
+        || (key == "endpoint" && !connector.eq_ignore_ascii_case(KINESIS_CONNECTOR))


I have no experience on aws config names used for connecting to kinesis (endpoint / kinesis.endpoint) vs names used for reading schema definition from s3 (endpoint_url). Would rely on @tabVersion with more experience to make sure the names are listed correctly, and with proper tests.

My previous comment used the word copy instead of move. I am not sure if some names in legacy with_properties are actually used by both with and format encode. Moving them away from with_properties may break things. Again, whether this is an real issue depends on the meaning / usage of specific names, which has been obscure for a long time every time we talk about with properties. Just bringing the idea up and would rely on your judgement.

xiangjinwu · 2023-12-14T09:10:28Z

src/connector/src/source/mod.rs

+    PREFIXES.iter().any(|prefix| key.starts_with(prefix))
+        || (key == "endpoint" && !connector.eq_ignore_ascii_case(KINESIS_CONNECTOR))


Studied serde rename and alias, and it turns out the only name shared by both is endpoint, which would not be moved in the case of kinesis. And existing kinesis connector are not using format encode from s3. (or we need to copy endpoint even when it is kinesis)

All other names for reading format encode from s3 has already been listed above properly.

Just for clarity, this is a concrete example of the current situation:

create [source | table] t with ( connector = 'kinesis', stream = 't', aws.region = 'r0', endpoint = 'e0', aws.credentials.access_key_id = 'ak0', aws.credentials.secret_access_key = 'sk0' ) format plain encode avro ( schema.location = 's3://foo/bar', region = 'r1', access_key = 'ak1', secret_key = 'sk1' );

It is unfortunate we seem to have no test or doc example about reading schema definition from s3.

xiangjinwu · 2023-12-14T09:15:40Z

Built an image with IMAGE_TAG=pr-13762-fix-prop-option-misuse
https://buildkite.com/risingwavelabs/docker/builds/14892
Then ran integration tests with RW_IMAGE_TAG=pr-13762-fix-prop-option-misuse
https://buildkite.com/risingwavelabs/integration-tests/builds/514#018c6776-3b56-4406-b449-55cab832dae0

Rossil2012 added 2 commits November 27, 2023 17:15

fix duplicate keys of AwsAuth in props and options

08ffe34

not merge options into props

d46b94c

github-actions bot added the type/fix Bug fix label Dec 1, 2023

Rossil2012 added 4 commits December 4, 2023 11:20

fix handle_create_source

a92d67f

Merge branch 'kanzhen/fix-options-props' into kanzhen/fix-prop-option…

08bda7f

…-misuse

chores

da9ce5c

consume aws options

1295a58

Rossil2012 marked this pull request as ready for review December 4, 2023 04:15

fmt

b19fe0b

Rossil2012 added 3 commits December 4, 2023 13:58

add PbSource.options

0d28278

persist options in meta

f9092b9

unit test

a914777

Rossil2012 requested review from tabVersion, xiangjinwu, xxchan and yezizp2012 and removed request for tabVersion and xiangjinwu December 4, 2023 09:21

xxchan reviewed Dec 4, 2023

View reviewed changes

xxchan mentioned this pull request Dec 4, 2023

fix: duplicate keys of AwsAuth in props and options #13666

Closed

9 tasks

Rossil2012 self-assigned this Dec 5, 2023

BugenZhao reviewed Dec 6, 2023

View reviewed changes

xxchan mentioned this pull request Dec 6, 2023

test(backwards-compat): test invalid WITH options #13824

Merged

9 tasks

kwannoel added the ci/run-backwards-compat-tests Run backwards compatibility tests in your PR. label Dec 6, 2023

Rossil2012 added 2 commits December 11, 2023 12:14

fix backward compatibility

eae190d

fix SCHEMA_REGISTRY_USERNAME/PASSWORD

e28db0f

yezizp2012 reviewed Dec 11, 2023

View reviewed changes

src/meta/src/manager/catalog/database.rs Outdated Show resolved Hide resolved

fix backward compatibility in CatalogManager

213f6b5

fuyufjh requested review from tabVersion, xiangjinwu, yezizp2012, BugenZhao, xxchan and st1page December 12, 2023 06:58

tabVersion approved these changes Dec 12, 2023

View reviewed changes

proto/catalog.proto Show resolved Hide resolved

src/connector/src/source/mod.rs Show resolved Hide resolved

fuyufjh reviewed Dec 12, 2023

View reviewed changes

dashboard/pages/api/streaming.ts Outdated Show resolved Hide resolved

fuyufjh reviewed Dec 12, 2023

View reviewed changes

Rossil2012 added 2 commits December 13, 2023 13:56

Merge branch 'main' into kanzhen/fix-prop-option-misuse

b70b1d8

fix AwsAuthProps in options

3135e9f

Rossil2012 commented Dec 13, 2023

View reviewed changes

dashboard: remove properties in type Relation

9f817c3

xiangjinwu reviewed Dec 14, 2023

View reviewed changes

xiangjinwu approved these changes Dec 14, 2023

View reviewed changes

Rossil2012 added 2 commits December 15, 2023 10:54

copy to format_encode_options instead of move

185a590

Merge branch 'main' into kanzhen/fix-prop-option-misuse

47e6ff3

Rossil2012 added this pull request to the merge queue Dec 15, 2023

Merged via the queue into main with commit c253bd3 Dec 15, 2023
28 of 30 checks passed

Rossil2012 deleted the kanzhen/fix-prop-option-misuse branch December 15, 2023 06:50

xiangjinwu mentioned this pull request Jan 23, 2024

bug(source): certain properties/options missing after upgrading from v1.5.0 to v1.6.0 #14755

Closed

xxchan mentioned this pull request Mar 20, 2024

Reject FORMAT ENCODE options in WITH options #15824

Open

		let properties = WithOptions::new(prost.properties.clone()).into_inner();
		let options = WithOptions::new(prost.options.clone()).into_inner();

	pub struct ConnectorSchema {
	pub format: Format,
	pub row_encode: Encode,
	pub row_options: Vec<SqlOption>,
	}

		with_properties: &HashMap<String, String>,
		row_options: &mut BTreeMap<String, String>,

	fn consume_aws_config_from_options(format_encode_options: &mut BTreeMap<String, String>) {
	fn remove_aws_config_from_options(format_encode_options: &mut BTreeMap<String, String>) {

		PREFIXES.iter().any(\|prefix\| key.starts_with(prefix))
		\|\| (key == "endpoint" && !connector.eq_ignore_ascii_case(KINESIS_CONNECTOR))

fix: misuse of props and options in CREATE SOURCE #13762

fix: misuse of props and options in CREATE SOURCE #13762

Conversation

Rossil2012 commented Dec 1, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

codecov bot commented Dec 4, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

xxchan Dec 4, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

st1page Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

tabVersion Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

xxchan 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

xxchan commented Dec 4, 2023

Rossil2012 commented Dec 5, 2023 • 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

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

tabVersion commented Dec 10, 2023

tabVersion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuyufjh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangjinwu Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

xiangjinwu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangjinwu Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

xiangjinwu commented Dec 14, 2023

fix: misuse of props and options in `CREATE SOURCE` #13762

fix: misuse of props and options in `CREATE SOURCE` #13762

Rossil2012 commented Dec 1, 2023 •

edited

Loading

codecov bot commented Dec 4, 2023 •

edited

Loading

xxchan Dec 4, 2023 •

edited

Loading

st1page Dec 6, 2023 •

edited

Loading

tabVersion Dec 8, 2023 •

edited

Loading

Rossil2012 commented Dec 5, 2023 •

edited

Loading

xiangjinwu Dec 14, 2023 •

edited

Loading

xiangjinwu Dec 14, 2023 •

edited

Loading