-
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
fix(source/nats): add missing default
attribute for vectors
#18261
fix(source/nats): add missing default
attribute for vectors
#18261
Conversation
6d6a6a1
to
8df4a9a
Compare
required: false | ||
default: Default::default |
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.
Does this PR imply the following problems:
- Without the
serde(default)
therequired: false
here is actually misleading - Other attributes without
serde(default)
have the same problem
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.
Based on our testing of this, the other attributes are unaffected. It seems that only the vector types are affected - my guess is that this is somehow needed due to the custom deserializer.
I drew reference from the Iceberg sink that uses a similar pattern
risingwave/src/connector/src/sink/iceberg/mod.rs
Lines 129 to 141 in 63b4d47
#[serde( | |
rename = "s3.path.style.access", | |
default, | |
deserialize_with = "deserialize_bool_from_string" | |
)] | |
pub path_style_access: bool, | |
#[serde( | |
rename = "primary_key", | |
default, | |
deserialize_with = "deserialize_optional_string_seq_from_string" | |
)] | |
pub primary_key: Option<Vec<String>>, |
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.
Thanks! Yes, it seems only required when using deserialize_with
. We will see if we can fix the required: false
in yaml.
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.
Anyway just to confirm, these attributes are optional so I think it's fine to keep required: false
:)
Thanks for taking a look!
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.
Yes these 2 are now truly optional with default
. I meant fixing the yaml generator when default
is missing but deserialize_with
is present.
7998622
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Currently, the
backoff
andfilter_subjects
fields are required due to a missingdefault
attribute, causing source creation to fail when they are not defined.However, these fields should are supposed to be optional and should allow the creation of the source to proceed, even if they are not defined.
This PR adds the missing
default
attribute, which fixes the above issue.Related to #17615
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.