refactor(source): refactor s3 source `WITH` options handling #14190

xxchan · 2023-12-25T08:46:13Z

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

What's changed and what's your intention?

remove the if in extract

IIUC, previously we need the if here because s3_v2 and s3 shares the same config struct, and thus the macro cannot dispatch correctly. Since we have separated types now, the special treatment seems unnecessary and confusing.
Share a S3PropertiesCommon, instead of the top-level S3Properties. This corresponds to the pattern described in feat(source): deny unknown fields for WITH options #13654 (comment) . (We need to add unknown_fields in both the top-level structs later..)

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 · 2023-12-25T08:46:24Z

Current dependencies on/for this PR:

main
- PR refactor(source): remove unnecessary Box for NexmarkProperties #14183
  - PR refactor(source): refactor s3 source WITH options handling #14190 👈
    - PR refactor(connector): Add a dummy trait WithOptions #14175
      - PR feat(source): deny unknown fields for WITH options #13654

This stack of pull requests is managed by Graphite.

xxchan · 2023-12-25T11:30:40Z

😟

xxchan · 2023-12-25T11:35:33Z

Wait, it seems this is also failing on main...

wcy-fdu · 2023-12-26T03:32:59Z

Hmm... I also find that currently both s3 and s3_v2 can not work, and can be reproduced locally. I suspect it's a bug introduced by some recent PRs, any ideas? cc @tabVersion

tabVersion · 2023-12-26T06:54:28Z

suspect #13707 breaks s3 or parser parts, may suspend the pr and wait for @Rossil2012 find out why.

#14196

xxchan · 2023-12-26T07:26:45Z

It seems we can't land #13654 in 1.6

xxchan · 2023-12-27T05:02:24Z

s3 source tests passed @tabVersion

https://buildkite.com/risingwavelabs/main-cron/builds/1415

tabVersion · 2023-12-27T09:21:53Z

src/connector/src/source/base.rs

-            match connector.as_str() {
-                "s3_v2" => {
-                    let assume_role = with_properties.get("s3.assume_role").cloned();
-                    return Ok(ConnectorProperties::OpendalS3(Box::new(
-                        OpendalS3Properties {
-                            s3_properties: S3Properties::try_from_hashmap(with_properties)?,
-                            assume_role,
-                        },
-                    )));
-                }
-                "gcs" => {
-                    return Ok(ConnectorProperties::Gcs(Box::new(
-                        GcsProperties::try_from_hashmap(with_properties)?,
-                    )));
-                }


where's the dispatch logic going

... Do you remember why the dispatch is needed here?

IIUC, previously we need the if here because s3_v2 and s3 shares the same config struct, and thus the macro cannot dispatch correctly. Since we have separated types now, the special treatment seems unnecessary and confusing.

I mentioned in the PR description

match_source_name_str takes care of all cases, which I think goes to this in the lowest level

risingwave/src/connector/src/macros.rs

Lines 35 to 37 in 4695ad1

{ S3, $crate::source::filesystem::S3Properties, $crate::source::filesystem::FsSplit },

{ Gcs, $crate::source::filesystem::opendal_source::GcsProperties , $crate::source::filesystem::OpendalFsSplit<$crate::source::filesystem::opendal_source::OpendalGcs> },

{ OpendalS3, $crate::source::filesystem::opendal_source::OpendalS3Properties, $crate::source::filesystem::OpendalFsSplit<$crate::source::filesystem::opendal_source::OpendalS3> },

thanks for helping me remember the structure. No more question.

tabVersion

rest LGTM

xxchan mentioned this pull request Dec 25, 2023

refactor(connector): Add a dummy trait WithOptions #14175

Merged

9 tasks

xxchan mentioned this pull request Dec 25, 2023

refactor(source): remove unnecessary Box for NexmarkProperties #14183

Merged

9 tasks

github-actions bot added the type/refactor label Dec 25, 2023

xxchan added ci/run-s3-source-tests ci/run-e2e-source-tests ci/run-main-cron and removed ci/run-e2e-source-tests ci/run-s3-source-tests labels Dec 25, 2023

xxchan requested review from tabVersion and wcy-fdu December 25, 2023 11:38

xxchan force-pushed the 12-25-refactor_source_remove_unnecessary_Box_for_NexmarkProperties branch from c33129f to 64b9ed0 Compare December 25, 2023 12:13

xxchan force-pushed the 12-25-refactor_source_unify_s3_v2_handling_in_ConnectorProperties_extract branch from d6e8b64 to 1ff6db4 Compare December 25, 2023 12:13

xxchan mentioned this pull request Dec 25, 2023

feat(source): deny unknown fields for WITH options #13654

Merged

9 tasks

xxchan changed the title ~~refactor(source): unify s3_v2 handling in ConnectorProperties::extract~~ refactor(source): refactor s3 source WITH options handling Dec 25, 2023

Base automatically changed from 12-25-refactor_source_remove_unnecessary_Box_for_NexmarkProperties to main December 26, 2023 04:34

xxchan force-pushed the 12-25-refactor_source_unify_s3_v2_handling_in_ConnectorProperties_extract branch from 71d2b0f to ecba13b Compare December 26, 2023 05:10

xxchan added 2 commits December 27, 2023 11:14

refactor(source): unify s3_v2 handling in ConnectorProperties::extract

a7b7bab

extract a common struct for s3 sources

e44b49f

xxchan force-pushed the 12-25-refactor_source_unify_s3_v2_handling_in_ConnectorProperties_extract branch from ecba13b to e44b49f Compare December 27, 2023 03:14

xxchan requested review from tabVersion and wcy-fdu and removed request for tabVersion and wcy-fdu December 27, 2023 05:02

xxchan added ci/run-build ci/main-cron/skip-ci ci/run-s3-source-tests labels Dec 27, 2023

tabVersion reviewed Dec 27, 2023

View reviewed changes

tabVersion approved these changes Dec 27, 2023

View reviewed changes

xxchan added this pull request to the merge queue Dec 27, 2023

Merged via the queue into main with commit 476daec Dec 27, 2023
39 of 41 checks passed

xxchan deleted the 12-25-refactor_source_unify_s3_v2_handling_in_ConnectorProperties_extract branch December 27, 2023 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(source): refactor s3 source `WITH` options handling #14190

refactor(source): refactor s3 source `WITH` options handling #14190

xxchan commented Dec 25, 2023 •

edited

Loading

xxchan commented Dec 25, 2023 •

edited

Loading

xxchan commented Dec 25, 2023

xxchan commented Dec 25, 2023

wcy-fdu commented Dec 26, 2023

tabVersion commented Dec 26, 2023 •

edited by xxchan

Loading

xxchan commented Dec 26, 2023

xxchan commented Dec 27, 2023 •

edited

Loading

tabVersion Dec 27, 2023

xxchan Dec 27, 2023

xxchan Dec 27, 2023

xxchan Dec 27, 2023

xxchan Dec 27, 2023

tabVersion Dec 27, 2023

tabVersion left a comment

	{ S3, $crate::source::filesystem::S3Properties, $crate::source::filesystem::FsSplit },
	{ Gcs, $crate::source::filesystem::opendal_source::GcsProperties , $crate::source::filesystem::OpendalFsSplit<$crate::source::filesystem::opendal_source::OpendalGcs> },
	{ OpendalS3, $crate::source::filesystem::opendal_source::OpendalS3Properties, $crate::source::filesystem::OpendalFsSplit<$crate::source::filesystem::opendal_source::OpendalS3> },

refactor(source): refactor s3 source WITH options handling #14190

refactor(source): refactor s3 source WITH options handling #14190

Conversation

xxchan commented Dec 25, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

xxchan commented Dec 25, 2023 • edited Loading

xxchan commented Dec 25, 2023

xxchan commented Dec 25, 2023

wcy-fdu commented Dec 26, 2023

tabVersion commented Dec 26, 2023 • edited by xxchan Loading

xxchan commented Dec 26, 2023

xxchan commented Dec 27, 2023 • edited Loading

tabVersion Dec 27, 2023

Choose a reason for hiding this comment

xxchan Dec 27, 2023

Choose a reason for hiding this comment

xxchan Dec 27, 2023

Choose a reason for hiding this comment

xxchan Dec 27, 2023

Choose a reason for hiding this comment

xxchan Dec 27, 2023

Choose a reason for hiding this comment

tabVersion Dec 27, 2023

Choose a reason for hiding this comment

tabVersion left a comment

Choose a reason for hiding this comment

refactor(source): refactor s3 source `WITH` options handling #14190

refactor(source): refactor s3 source `WITH` options handling #14190

xxchan commented Dec 25, 2023 •

edited

Loading

xxchan commented Dec 25, 2023 •

edited

Loading

tabVersion commented Dec 26, 2023 •

edited by xxchan

Loading

xxchan commented Dec 27, 2023 •

edited

Loading