AVRO-1695: Ruby support for logical types #44
Conversation
dasch
changed the title
| end | ||
| end | ||
|
|
||
| class Null |
There was a problem hiding this comment.
would "Identity" or something similar be easier for new folks to recognize?
|
I hope to have more time to work on this next week. |
dasch
force-pushed
the
dasch/ruby-logical-types
branch
2 times, most recently
from
9b78251 to
88d7bb1
Compare
|
@busbey would you like to review this? |
| real_parse(MultiJson.load(json_string), {}) | ||
| json_obj = MultiJson.load(json_string) | ||
| schema = real_parse(json_obj, {}) | ||
| schema.logical_type = LogicalTypes.parse(json_obj) if json_obj.is_a?(Hash) |
There was a problem hiding this comment.
I think it might make more sense for this to be added to the real_parse method. The handling for when the json_obj is a different type is already built in and you wouldn't have to expose :logical_type=.
There was a problem hiding this comment.
You mean pass the logical type to the inializer when instantiating the Schema object? I tried that, but it would require changing the code quite a bit – the real_parse method has multiple returns and separately instantiates the classes. Also, all of the Schema subclasses' initialize methods would have to be changed. If you're okay with the larger diff I can do it.
At this point, though, I'd prefer extracting a separate SchemaParser class to handle the parsing bits in order to keep the code cleaner. I originally didn't want to do this because I preferred the PR to be simple and focused. If you're also okay with it I can refactor a bit.
There was a problem hiding this comment.
I see what you're saying about the difficulty of doing it that way, which is pretty annoying. But, I think it would be better to avoid exposing the setter.
There was a problem hiding this comment.
Ok, I'll take a look at changing it next week.
On lør. 5. sep. 2015 at 02.07 Ryan Blue [email protected] wrote:
In lang/ruby/lib/avro/schema.rb
#44 (comment):@@ -33,7 +35,10 @@ class Schema
LONG_MAX_VALUE = (1 << 63) - 1def self.parse(json_string)
real_parse(MultiJson.load(json_string), {})json_obj = MultiJson.load(json_string)schema = real_parse(json_obj, {})schema.logical_type = LogicalTypes.parse(json_obj) if json_obj.is_a?(Hash)I see what you're saying about the difficulty of doing it that way, which
is pretty annoying. But, I think it would be better to avoid exposing the
accessor.—
Reply to this email directly or view it on GitHub
https://github.com/apache/avro/pull/44/files#r38804034.
|
Thanks @dasch! This is looking pretty close to being ready. |
Only logical types that map directly to Ruby standard library types are supported: * `date` maps to Date; * `timestamp-millis` and `timestamp-micros` map to Time.
dasch
force-pushed
the
dasch/ruby-logical-types
branch
from
88d7bb1 to
e61b78a
Compare
|
@dasch, is this ready for another round of review? I was going through issues and realized we haven't closed this out. I can take another look if you'd like. |
|
@rdblue I actually haven't looked at this for a while – I got sidetracking doing other stuff. If someone would adopt and finish the PR I'd be ❤️ |
|
@dasch, we're still interested in getting this in. It was just automatically closed by moving the project to git from SVN. You should be able to reopen it or submit a new one to get this change in. Thanks! |
|
@rdblue only project maintainers can re-open issues. I probably won't have time to look at this for the foreseeable future, but you're free to take it over and finish it. |
|
I've attempted to take this over in #116. |
Add Ruby support for logical types
Only logical types that map directly to Ruby standard library types are supported:
datemaps to Date;timestamp-millisandtimestamp-microsmap to Time.The remaining types are difficult to cleanly map to Ruby types, so I've refrained from doing so. Furthermore, there's no direct support for plugging in custom mappers – I'm unsure if this is needed.