-
Notifications
You must be signed in to change notification settings - Fork 590
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
feat: schemaless ingestion for encode json
(INCLUDE payload
)
#18437
Conversation
Signed-off-by: tabVersion <tabvision@bupt.icu>
encode jsod
encode json
Signed-off-by: tabVersion <tabvision@bupt.icu>
Signed-off-by: tabVersion <tabvision@bupt.icu>
Signed-off-by: tabVersion <tabvision@bupt.icu>
encode json
encode json
(INCLUDE payload
)
Co-authored-by: xxchan <xxchan22f@gmail.com>
…labs/risingwave into tab/schemaless-ingest
Signed-off-by: tabVersion <tabvision@bupt.icu>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Cool!
// remove s3 for no longer use | ||
// (S3_CONNECTOR, HashSet::from(["file", "offset"])), | ||
( | ||
OPENDAL_S3_CONNECTOR, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems currently we only have include
tests for Kafka. Would you minding also adding a test for s3? Since s3 connector has quite some bugs, I'm a little worried about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd merge this one first and do the test in following pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -646,6 +646,7 @@ impl<'a> JsonAccess<'a> { | |||
impl Access for JsonAccess<'_> { | |||
fn access<'a>(&'a self, path: &[&str], type_expected: &DataType) -> AccessResult<DatumCow<'a>> { | |||
let mut value = &self.value; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
// hack here: Get the whole payload as a single column | ||
// use a special mark empty slice as path to represent the whole payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think empty path is not "hack" at all, but just reasonable usage?
// hack here: Get the whole payload as a single column | |
// use a special mark empty slice as path to represent the whole payload | |
// access empty path to get the whole payload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is an undefined behavior in prev impl, we always expect path
a non-empty slice.
Co-authored-by: xxchan <xxchan22f@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we don’t have NATS here? Saw a user asking about it
https://risingwave-community.slack.com/archives/C02T3F7UYM6/p1727303064979409?thread_ts=1727292740.494619&channel=C02T3F7UYM6&message_ts=1727303064.979409
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
resolve #12207
Checklist
./risedev check
(or alias,./risedev c
)Documentation
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.