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

feat: allow array block contain only length #265

Merged
merged 3 commits into from
Jun 8, 2023

Conversation

meandnano
Copy link
Contributor

config.go Outdated Show resolved Hide resolved
@meandnano
Copy link
Contributor Author

One question for another potential PR. Right now there's no way to configure avro.Writer inside ocf.Encoder. I guess it is possible to add writerConfig API field to ocf.encoderConfig with corresponding EncoderFunc and use this config while constructing avro.Writer inside ocf.NewEncoder.

@nrwiersma
Copy link
Member

I assume it is to expose this option, in which case it would be added like WithBlockLength.

Copy link
Member

@nrwiersma nrwiersma left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@nrwiersma nrwiersma merged commit 88b071f into hamba:main Jun 8, 2023
@meandnano
Copy link
Contributor Author

meandnano commented Jun 8, 2023

I assume it is to expose this option, in which case it would be added like WithBlockLength.

I gave it another thought - ocf.WithBlockLength controls block length of the OCF file which is not the same as array/map block. And it is a feature of ocf.Encoder that logically resides above avro.Writer that is responsible for actual data encoding. Moreover, adding WithDisableBlockSizeHeader next to WithBlockLength in the ocf package might indeed be confusing.

Another point is that apart from DisableBlockSizeHeader there are other config fields which right now are locked by avro.DefaultConfig when writing OCF files.

Adding func WithWriterConfig(cfg API) EncoderFunc to ocf will allow to adjust any of them and thus have configurable encoder for OCF just as it is for plain messages.

What do you think?

UPD: Just realized the issue, API is in avro package and therefore cannot be accessed in ocf package without substantial refactoring. However ocf.WithWriter function seems like could be feasible solution... (tired at the end of working day)

@nrwiersma
Copy link
Member

Sounds good to me

@meandnano
Copy link
Contributor Author

Great, I can make PR tomorrow

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

Successfully merging this pull request may close these issues.

3 participants