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

add support for logicalType #10

Closed
carlos-markavip opened this issue Nov 2, 2015 · 8 comments
Closed

add support for logicalType #10

carlos-markavip opened this issue Nov 2, 2015 · 8 comments

Comments

@carlos-markavip
Copy link

It would already be pretty useful to preserve logicalType as well as extra attributes when parsing the schema and when writing it back in a container header.

@mtth
Copy link
Owner

mtth commented Nov 2, 2015

Good idea. Will include this in a future release.

@carlos-markavip
Copy link
Author

OK, thanks for the quick reply. I'm in a hurry so I'll go ahead and implement this for my needs.
If you've got some thoughts about how you'd like to see this written I'd love to read them. That would improve the odds of contributing some generally useful code.

@mtth
Copy link
Owner

mtth commented Nov 3, 2015

I'd like to think some more about how best to implement logical types, no definite ideas yet. I'm happy to review your code if you submit a pull request.

For your use-case, I wonder if being able to set the schema written by the BlockEncoder would be sufficient? If so, it would be straightforward to add a new key (e.g. schema) to its constructor's options, defaulting to the type's canonical schema.

@carlos-markavip
Copy link
Author

For my current use case that would probably be enough for now, yes

@mtth
Copy link
Owner

mtth commented Nov 3, 2015

Great, let's start with that then.

And come to think of it, it would be even better to change the first argument of BlockEncoder (and related functions) to accept a schema rather than only a type. The behavior would be consistent with parse, backwards compatible, and save adding a new option (along with the necessary compatibility check that would have been required).

If you can wait until tomorrow morning, I'll get this change out then.

@carlos-markavip
Copy link
Author

Tomorrow would be great, thanks. I've made the change you suggested locally already, and it will be trivial the change the calling code again.

@mtth
Copy link
Owner

mtth commented Nov 3, 2015

Done in df20230 and released in 2.4.0.

@carlos-markavip
Copy link
Author

works for me, thanks

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

No branches or pull requests

2 participants