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

AVRO-1695: Ruby support for logical types revisited #116

Merged
merged 2 commits into from
Nov 7, 2018

Conversation

tjwp
Copy link
Contributor

@tjwp tjwp commented Aug 23, 2016

This is a continuation of the changes originally submitted in #44 for https://issues.apache.org/jira/browse/AVRO-1695.

Original description:

Add Ruby support for logical types

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.

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.

I've tried to add address the feedback from the original PR, primarily pushing some of the implementation into Avro::Schema.real_parse and ensuring that the Ruby tests cover all the same cases as the Java conversion tests.

One change I've made here is to have Avro::Schema#logical_type return the name of the logical type.

For now I've preserved the original changes as a separate commit, but I can rebase the changes if this gets accepted.

CC @dasch

@dasch
Copy link

dasch commented Aug 24, 2016

👍

@tjwp
Copy link
Contributor Author

tjwp commented Aug 24, 2016

Updated Manifest and changes to ensure that TimestampMicros works with DateTime.

@tjwp
Copy link
Contributor Author

tjwp commented Dec 5, 2016

@rdblue You reviewed the original pull request for this support in ruby. Is there anything that we can do to help push this along?

@rdblue
Copy link
Contributor

rdblue commented Dec 5, 2016

Sorry for the delay. I'll review this soon. I'm planning on making some time next weekend (17-18 Dec).

@busbey
Copy link
Contributor

busbey commented Jan 28, 2017

If you wouldn't mind rebasing to current master, I'll take a look at this my next time around (~2/7)

@tjwp tjwp force-pushed the tjwp/ruby-logical-types branch from 897fb3a to a743b1c Compare January 30, 2017 19:37
@tjwp
Copy link
Contributor Author

tjwp commented Jan 30, 2017

@busbey I've rebased this, and also squashed the commits. I'll also rebase #170, another ruby PR, against current master.

@tjwp
Copy link
Contributor Author

tjwp commented Apr 2, 2017

@spacharya I noticed that you recently looked at another Ruby PR to potentially be merged soon. Would you mind taking a look at this one too? We've been using this code in production for a while.

Copy link
Contributor

@busbey busbey left a comment

Choose a reason for hiding this comment

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

This looks like a great start to me. I'll merge as soon as I can run the ruby suite tests again. (currently blocked on AVRO-1516 and AVRO-2013)

@busbey
Copy link
Contributor

busbey commented Apr 2, 2017

I thought of another question: anyone know if we have any cross-language compatibility tests in share/ for logical types yet? If we do, I'd want to make sure we're good here. If not, I just want to make sure we have a follow-on somewhere for it.

@tjwp
Copy link
Contributor Author

tjwp commented Apr 4, 2017

I'll resolve the conflicts on this tomorrow. Also, happy to help with the ruby suite tests if there's anything I can do there.

@tjwp tjwp force-pushed the tjwp/ruby-logical-types branch from a743b1c to 3973888 Compare April 4, 2017 18:13
@tjwp
Copy link
Contributor Author

tjwp commented Apr 4, 2017

Rebased on the current master.

@tjwp
Copy link
Contributor Author

tjwp commented Apr 5, 2017

@busbey I took a look through share/ and there does not seem to be anything for logical types yet.

@tjwp
Copy link
Contributor Author

tjwp commented Apr 14, 2017

@busbey Any chance of getting this merged prior to the next 1.8.2 RC?

Copy link

@sldblog sldblog left a comment

Choose a reason for hiding this comment

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

❤️

@Fokko
Copy link
Contributor

Fokko commented Nov 7, 2018

@tjwp Can you rebase onto master?

@tjwp tjwp force-pushed the tjwp/ruby-logical-types branch from 3973888 to b409d89 Compare November 7, 2018 20:12
@tjwp
Copy link
Contributor Author

tjwp commented Nov 7, 2018

@Fokko rebased and tested with:

SUCCESS rvm: 1.9.3-p551
SUCCESS rvm: 2.0.0-p648
SUCCESS rvm: 2.1.10
SUCCESS rvm: 2.2.8
SUCCESS rvm: 2.3.8
SUCCESS rvm: 2.4.5
SUCCESS rvm: 2.5.3

@tjwp tjwp force-pushed the tjwp/ruby-logical-types branch from b409d89 to 02cc098 Compare November 7, 2018 20:56
@tjwp
Copy link
Contributor Author

tjwp commented Nov 7, 2018

Rebased again with a fix for the ruby tests and the additional change referenced here: salsify/avro-patches#18 (comment)

@Fokko
Copy link
Contributor

Fokko commented Nov 7, 2018

@tjwp Travis is still not happy, can you take a peek?

.................../testptch/unknown/lang/ruby/lib/avro/schema.rb:318: warning: too many arguments for format string
..........................F
===============================================================================
Failure: test_timestamp_micros_long_conversion(TestLogicalTypes)
/testptch/unknown/lang/ruby/test/test_logical_types.rb:83:in `test_timestamp_micros_long_conversion'
     80: 
     81:     now = Time.now.utc
     82: 
  => 83:     assert_equal now, type.decode(type.encode(now))
     84:     assert_equal 1432849613221843, type.encode(Time.utc(2015, 5, 28, 21, 46, 53, 221843))
     85:     assert_equal 1432849613221843, type.encode(DateTime.new(2015, 5, 28, 21, 46, 53.221843))
     86:     assert_equal Time.utc(2015, 5, 28, 21, 46, 53, 221843), type.decode(1432849613221843)
<2018-11-07 20:38:27 UTC> expected but was
<2018-11-07 20:38:27 UTC>

diff:
  2018-11-07 20:38:27 UTC
===============================================================================

@Fokko Fokko merged commit 33f7177 into apache:master Nov 7, 2018
@Fokko
Copy link
Contributor

Fokko commented Nov 7, 2018

Thanks @tjwp

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.

6 participants