-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Python Client] Fix handle complex schema #11400
[Python Client] Fix handle complex schema #11400
Conversation
Thanks for your contribution. For this PR, do we need to update docs? (The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks) |
obj = {} | ||
for k, v in d.items(): | ||
if isinstance(v, Record): | ||
obj[k] = self.encode_dict(v.__dict__) |
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.
This check could also be don in the _get_serialized_value()
method:
def _get_serialized_value(self, x):
if isinstance(x, enum.Enum):
return x.name
elif isinstance(x, Record):
self.encode_dict(x.__dict__)
else:
return x
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 fixed this. PTAL
@@ -56,15 +57,20 @@ def _get_fields(cls, dct): | |||
|
|||
class Record(with_metaclass(RecordMeta, object)): | |||
|
|||
def __init__(self, default=None, required_default=False, required=False, *args, **kwargs): | |||
def __init__(self, default=None, required_default=False, required=False, decode=False, *args, **kwargs): |
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's not easy to understand what is the meaning of decode=False|True
here. Also these arguments are exposed to the users (eg: marking a field as "required"), do we need to expose this decode
too?
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 this is not necessary, I remove the param decode
.
@gaoran10 Please rebase to the master branch to make the CI gets passed. |
377a12a
to
64a56c0
Compare
Fixes apache#7785, apache#11221 ### Motivation Currently, the Pulsar python client couldn't handle complex schema, users using complex schema will encounter errors. ### Modifications Fix AvroSchema encodes and decodes complex schema. Fix JsonSchema decodes complex schema. ### Verifying this change This change added tests and can be verified as follows: - *Encode and decode complex schema data* - *Produce and consume complex schema data*
Fixes #7785, #11221 ### Motivation Currently, the Pulsar python client couldn't handle complex schema, users using complex schema will encounter errors. ### Modifications Fix AvroSchema encodes and decodes complex schema. Fix JsonSchema decodes complex schema. ### Verifying this change This change added tests and can be verified as follows: - *Encode and decode complex schema data* - *Produce and consume complex schema data* (cherry picked from commit 2370643)
Fixes apache#7785, apache#11221 ### Motivation Currently, the Pulsar python client couldn't handle complex schema, users using complex schema will encounter errors. ### Modifications Fix AvroSchema encodes and decodes complex schema. Fix JsonSchema decodes complex schema. ### Verifying this change This change added tests and can be verified as follows: - *Encode and decode complex schema data* - *Produce and consume complex schema data*
Fixes #7785, #11221
Motivation
Currently, the Pulsar python client couldn't handle complex schema, users using complex schema will encounter errors.
Modifications
Fix AvroSchema encodes and decodes complex schema.
Fix JsonSchema decodes complex schema.
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changes