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

emit default values for non populated fields in protobuf #3022

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ealonsodb
Copy link

When you use schema_registry_decode processor, the protobuf default values (0 for int32, "" for string, false for bool, 0.0 for float, etc...) are not populated in the row.
With this new parameter you can have all those default values populated, no need to check for nulls after that processor

@CLAassistant
Copy link

CLAassistant commented Nov 20, 2024

CLA assistant check
All committers have signed the CLA.

@rockwotj rockwotj self-requested a review November 20, 2024 21:25
@rockwotj
Copy link
Collaborator

Thanks for this! I will review after Thanksgiving.

One quick note is it would be good if the configuration option was either nested in a protobuf_options object or if the field was prefixed with protobuf to make it more clear what it was for (also for alignment with avro). I personally prefer the former option

Copy link
Collaborator

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

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

Looks great thank you for adding tests! I have one request for the configuration to namespace the option under a protobuf field so that it's clear this only works for protobuf and we have a place for more protobuf related configs as we inevitably add more options here 😄

Field(service.NewURLField("url").Description("The base URL of the schema registry service."))
Field(service.NewURLField("url").
Description("The base URL of the schema registry service.")).
Field(service.NewBoolField("emit_default_values").
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we instead create a nested config object and add a boolean field for emit_defaults? I'd like to namespace these as over time I expect to have more protobuf knobs available.

That will look something like this:

service.NewObjectField("protobuf", service.NewBoolField("emit_defaults"))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants