Skip to content

feat(connector): support debezium mongo json - #9250

Merged
adevday merged 11 commits into
mainfrom
idx0dev/mongojsonparser
Apr 26, 2023
Merged

adevday merged 11 commits into
mainfrom
idx0dev/mongojsonparser

Conversation

@adevday

@adevday adevday commented Apr 18, 2023 •

Copy link
Copy Markdown
Contributor

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

What's changed and what's your intention?

A new row format DEBEZIUM_MONGO_JSON is added to our kakfa source. Thus we can load documents from MongoDB via Debezium.

Checklist For Contributors

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
  • All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

  • I have requested macro/micro-benchmarks as this PR can affect performance substantially, and the results are shown.

Documentation

  • My PR DOES NOT contain user-facing changes.

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

  • Connector (sources & sinks)
    A new row format DEBEZIUM_MONGO_JSON is added to our kakfa source.
    The source table schema has following constraints when using the DEBEZIUM_MONGO_JSON row format.
  • The table schema must have two columns, the one is called _id and the other is called payload. The MongoDB document's _id will be extracted into the _id column, and the whole document will be in payload column.
  • The payload column must be jsonb type.
  • The _id column must be the only primary key and its type is mapped. If the ObjectID is used as document's _id field type, the _id column should be varchar type. And If the int32 or int64 is used as document's _id field , the _id column should be int or bigint type.

Release note

  • Support Debezium for MongoDB

@adevday
adevday marked this pull request as draft April 18, 2023 08:54
@github-actions github-actions Bot added the type/feature Type: New feature. label Apr 18, 2023
Comment thread src/connector/src/parser/debezium/mongo_json_parser.rs Outdated
Comment thread src/connector/src/parser/debezium/mongo_json_parser.rs Outdated
@neverchanje

Copy link
Copy Markdown
Contributor

Did you test it end-to-end? What does the output look like?

@adevday
adevday marked this pull request as ready for review April 19, 2023 03:34
@adevday
adevday force-pushed the idx0dev/mongojsonparser branch from 09bb2af to 1ed5882 Compare April 19, 2023 09:01
@neverchanje neverchanje added the user-facing-changes Contains changes that are visible to users label Apr 21, 2023
@neverchanje

neverchanje commented Apr 21, 2023 •

Copy link
Copy Markdown
Contributor

Updated your PR description. I didn't find in the code where you handle the bson types. The point is, we need to ensure all types can be handled, whether or not they have the corresponding type in RisingWave. Generally, I recommend parsing the complex types, e.g. Regex, into VARCHAR, if we have no idea how to deal with them.

@tabVersion

tabVersion commented Apr 21, 2023 •

Copy link
Copy Markdown
Contributor

Updated your PR description. I didn't find in the code where you handle the bson types. The point is, we need to ensure all types can be handled, whether or not they have the corresponding type in RisingWave. Basically, I recommend parsing the complex types, e.g. Regex, into VARCHAR, if we have no idea how to deal with them.

bson is handled here doc. The method is used to convert bson types into some more widely used formats for json parser.
for types for which there is no common practice to deal with, the method will leave them to their original formats. As mentioned above, regex types will be parsed to varchar.
image

@neverchanje

neverchanje commented Apr 24, 2023 •

Copy link
Copy Markdown
Contributor

I see. I suggest handling the bson fields directly instead of converting them to json as the intermediate format, which is inefficient. If you want to temporarilly use the less efficient approach, please create an issue to record the future optimization opportunity.

match value {
  Bson::Double(v) => {
    ...
  },
  Bson::RegularExpression(v) => {
    ...
  },
}

@tabVersion

Copy link
Copy Markdown
Contributor

I see. I suggest handling the bson fields directly instead of converting them to json as the intermediate format, which is inefficient.

actually, we are storing jsonb type for the payload. As discussed offline, the schema for DEBEZIUM_MONGO_JSON is fixed to (_id <type>, payload jsonb) so it is inevitable to convert mongo CDC payload into json.
For datatype mapping, we only care about types in _id field and we don't have to cover all datatypes in MongoDB because not every type can be mongo's primary key.

@tabVersion

Copy link
Copy Markdown
Contributor

We will abandon the bson crate because it enables serde_json's preserve_order feature, which will disrupt key ordering when deser json. So we are handling _id deser manually without bson.

and now the expected behavior is

create table t ("_id" int, "payload" jsonb) with (...) row format DEBEZIUM_MONGO_JSON;

select (payload->'_id'->'$NumberLong')::int as _id from t;

@github-actions github-actions Bot removed the user-facing-changes Contains changes that are visible to users label Apr 25, 2023
DebeziumJson, // Keyword::DEBEZIUM_JSON
Json, // Keyword::JSON
DebeziumJson, // Keyword::DEBEZIUM_JSON
DebeziumMongoJson,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
DebeziumMongoJson,
DebeziumMongoJson, // DEBEZIUM_MONGO_JSON

@codecov

codecov Bot commented Apr 25, 2023 •

Copy link
Copy Markdown

Codecov Report

Merging #9250 (29b5d7e) into main (6f9c8e2) will decrease coverage by 0.04%.
The diff coverage is 49.85%.

@@            Coverage Diff             @@
##             main    #9250      +/-   ##
==========================================
- Coverage   70.74%   70.70%   -0.04%     
==========================================
  Files        1230     1231       +1     
  Lines      204838   205179     +341     
==========================================
+ Hits       144914   145074     +160     
- Misses      59924    60105     +181     
Flag Coverage Δ
rust 70.70% <49.85%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
src/connector/src/parser/mod.rs 48.52% <0.00%> (-0.84%) ⬇️
src/connector/src/source/base.rs 65.26% <ø> (ø)
src/source/src/source_desc.rs 55.11% <0.00%> (-0.44%) ⬇️
src/sqlparser/src/ast/statement.rs 64.20% <0.00%> (-0.26%) ⬇️
src/frontend/src/handler/create_source.rs 44.13% <1.56%> (-4.56%) ⬇️
...connector/src/parser/debezium/mongo_json_parser.rs 61.85% <61.85%> (ø)
src/connector/src/sink/kafka.rs 37.87% <100.00%> (+0.11%) ⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines +85 to +87
let int_str = obj["$numberInt"].as_str().unwrap_or_default();
Some(ScalarImpl::Int32(int_str.parse().unwrap_or_default()))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it safe to unwrap?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The value of field named $numberInt in a well-formed bson must be a string contains an integer. It's safe to unwrap as long as the source message is produced by mongoDB.

Comment thread src/connector/src/parser/debezium/mongo_json_parser.rs
Comment on lines +125 to +128
DataType::Jsonb
| DataType::Varchar
| DataType::Int32
| DataType::Int64

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any doc on key type limits?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could extend them if other _id type appears.

Comment thread src/connector/src/parser/debezium/mongo_json_parser.rs Outdated
Comment thread src/connector/src/parser/debezium/mongo_json_parser.rs Outdated
Comment on lines +377 to +395
if row_id_index.is_some() {
return Err(RwError::from(ProtocolError(
"Primary key must be specified when creating source with row format debezium."
.to_string(),
)));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why check row id here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For the case below.

CREATE TABLE mongo_customers (
	_id BIGINT, -- NO PRIMARY KEY HERE
	payload jsonb
) ...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

actually, we extract _id to make it primary key

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If the user leaves the CREATE TABLE xx (...) blank, we will create _id column and make it be primary key, otherwise we will not help users to create columns.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the user leaves the CREATE TABLE xx (...) blank, we will create _id column and make it be primary key

what is the type for _id column?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If the user leaves the CREATE TABLE xx (...) blank, we will create _id column and make it be primary key

what is the type for _id column?

varchar. Intended to be mapped from ObjectID, which is the default type for the _id field of a mongoDB docment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The important detail, however, is that you can also pass your own _id when inserting a document and it doesn't have to be of type ObjectId. In fact, according to MongoDB document specification, it can be of any type except arrays
from: https://stackoverflow.com/questions/51322231/what-is-the-type-of-id-field-in-mongo-database

_id may not always be a verchar.

@adevday
adevday force-pushed the idx0dev/mongojsonparser branch from 4e4dd59 to 752b813 Compare April 25, 2023 11:38

@tabVersion tabVersion left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rest LGTM

note that for this case:
_id MUST be the primary key and the user MUST annotate its type

it would be better to give a type mapping table to demonstrate how to correctly write _id type.

@adevday
adevday added this pull request to the merge queue Apr 26, 2023
@adevday

adevday commented Apr 26, 2023

Copy link
Copy Markdown
Contributor Author

rest LGTM

note that for this case: _id MUST be the primary key and the user MUST annotate its type

it would be better to give a type mapping table to demonstrate how to correctly write _id type.

updated in PR comment.

Merged via the queue into main with commit 337b7e3 Apr 26, 2023
@adevday
adevday deleted the idx0dev/mongojsonparser branch April 26, 2023 01:24
@tabVersion tabVersion added the user-facing-changes Contains changes that are visible to users label Apr 26, 2023
@github-actions github-actions Bot removed the user-facing-changes Contains changes that are visible to users label Apr 26, 2023
@tabVersion tabVersion mentioned this pull request Apr 26, 2023
6 of 7 tasks
@xxchan xxchan added the user-facing-changes Contains changes that are visible to users label May 11, 2023
@CharlieSYH CharlieSYH added the 📖✓ Covered or will be covered in the user docs. label May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/feature Type: New feature. user-facing-changes Contains changes that are visible to users 📖✓ Covered or will be covered in the user docs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants