Skip to content
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

refactor(source): minor refactors on source parser #17184

Merged
merged 7 commits into from
Jun 24, 2024

Conversation

xxchan
Copy link
Member

@xxchan xxchan commented Jun 10, 2024

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

What's changed and what's your intention?

mostly cleanup

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.

Copy link
Member Author

xxchan commented Jun 10, 2024

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

Join @xxchan and the rest of your teammates on Graphite Graphite

@xxchan xxchan marked this pull request as ready for review June 10, 2024 06:40
@xxchan xxchan force-pushed the refactor_move_json_schema_to_codec_crate branch from 3730c60 to d2bb979 Compare June 10, 2024 06:42
@xxchan xxchan requested a review from a team as a code owner June 10, 2024 06:42
@xxchan xxchan mentioned this pull request Jun 11, 2024
9 tasks
Base automatically changed from 06-05-feat_source_support_json_schema_addtionalproperties_map_ to main June 13, 2024 07:05
@xxchan xxchan force-pushed the refactor_move_json_schema_to_codec_crate branch from d2bb979 to 47e928b Compare June 18, 2024 00:34
@xxchan xxchan changed the title refactor: move json schema to codec crate refactor(source): move json schema to codec crate Jun 18, 2024
@xxchan xxchan force-pushed the refactor_move_json_schema_to_codec_crate branch 2 times, most recently from e1e5cac to cf04ecd Compare June 20, 2024 08:59
@xxchan xxchan changed the title refactor(source): move json schema to codec crate refactor(source): minor refactors on source parser Jun 20, 2024
Comment on lines 46 to 63
pub use risingwave_pb::plan_common::ColumnDesc;
pub type RisingWaveSchema = Vec<ColumnDesc>;
pub use apache_avro::schema::Schema as AvroSchema;
pub struct JsonSchema(pub serde_json::Value);
impl JsonSchema {
pub fn parse_str(schema: &str) -> anyhow::Result<Self> {
use anyhow::Context;

let value = serde_json::from_str(schema).context("failed to parse json schema")?;
Ok(Self(value))
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

exposed these while developing codec-console.

@xxchan xxchan requested review from BugenZhao and xiangjinwu June 20, 2024 09:03
Comment on lines -128 to +130
sea-schema = { version = "0.14", features = ["default", "sqlx-postgres", "sqlx-mysql"] }
sea-schema = { version = "0.14", default-features = false, features = [
"discovery",
"sqlx-postgres",
"sqlx-mysql",
] }
Copy link
Member Author

Choose a reason for hiding this comment

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

This is changed because I found cargo check -p risingwave_connector compile failed.

@BugenZhao
Copy link
Member

  • delete unused update op

I would suggest keeping it. There's no problem at all for us to generate an Update record for formats like Debezium (and it was before the unified parser refactoring). Compared to the current approach to use Upsert everywhere, a retractable record with correct old value can be beneficial in some places I guess.

src/connector/codec/src/lib.rs Outdated Show resolved Hide resolved
src/connector/codec/src/decoder/json/mod.rs Outdated Show resolved Hide resolved
@xxchan xxchan requested a review from BugenZhao June 21, 2024 04:03
@xxchan xxchan force-pushed the refactor_move_json_schema_to_codec_crate branch from 8baca40 to c28b986 Compare June 24, 2024 01:44
let avro_schema = convert_avro(json_schema, Context::default()).to_string();
let schema = Schema::parse_str(&avro_schema).context("failed to parse avro schema")?;
avro_schema_to_column_descs(&schema, Some(MapHandling::Jsonb)).map_err(Into::into)
json_schema.json_schema_to_columns().map_err(Into::into)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: json_schema_ in method name feels redundant.

@xxchan xxchan added this pull request to the merge queue Jun 24, 2024
Merged via the queue into main with commit 9c73c30 Jun 24, 2024
30 of 31 checks passed
@xxchan xxchan deleted the refactor_move_json_schema_to_codec_crate branch June 24, 2024 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants