Support serializing bigint as extension types #244

casperisfine · 2022-01-27T10:57:12Z

factory = MessagePack::Factory.new
factory.register_type(
  0x42,
  Integer,
  packer: ->(int) { int.to_s },
  unpacker: -> (payload) { Integer(payload) }
)

factory.dump(42) # Use the native integer type
factory.dump(2**400) # Use the registered packer

ext/msgpack/packer_ext_registry.h

tagomoris · 2022-01-28T01:02:15Z

The motivation about BigInt is acceptable for me.
But it's not clear to me why Integer is registered as an ext type but 42 should not be encoded into an ext type binary. Can you explain it clearly to users of msgpack-ruby?

In other words, 2 ** 400 is an Integer, (2**400).is_a?(Integer) is true and (2**400).class is Integer. How can we know whether it should be encoded as int or ext 0x42 (in your sample code)?

casperisfine · 2022-01-28T09:47:33Z

But it's not clear to me why Integer is registered as an ext type but 42 should not be encoded into an ext type binary. Can you explain it clearly to users of msgpack-ruby?

Of course. The rationale is that using the native type when the integer fits in it is:

more performant.
backward compatible.

IMO extension types only make sense if the data doesn't fit a native type, and I see no benefit into being able to replace native types.

We could however have it has an option, but again, I'd rather not do it unless someone see a use case for it (I don't).

tagomoris · 2022-02-08T14:05:46Z

@casperisfine Could you rebase this change too?
(Sorry, I'll see other pull-requests tomorrow or later)

casperisfine · 2022-02-08T14:15:17Z

Rebased.

(Sorry, I'll see other pull-requests tomorrow or later)

No need to apologize, thanks for all the reviews!

casperisfine

@tagomoris I updated this PR with your comments from #243

casperisfine · 2022-02-14T12:43:28Z

ext/java/org/msgpack/jruby/Encoder.java

+        if (hasBigintExtType && tryAppendWithExtTypeLookup(object)) {
+          return;
+        }
+        throw runtime.newRangeError(String.format("Cannot pack big integer: %s", value));


The MRI extension raises RangeError on bigint, but the JRuby one ArgumentError. I assume it is a bug.

casperisfine · 2022-02-14T12:43:28Z

ext/msgpack/factory_class.c

@@ -209,6 +211,10 @@ static VALUE Factory_register_type(int argc, VALUE* argv, VALUE self)
        }
    }

+    if (ext_module == rb_cInteger && RB_TEST(rb_hash_aref(options, ID2SYM(rb_intern("bigint"))))) {


No idea what's the best key to pass, so I went with bigint: true, but I'm open to suggestions.

casperisfine · 2022-02-14T12:43:28Z

lib/msgpack/bigint.rb

+# frozen_string_literal: true
+
+module MessagePack
+  module Bigint


I think it makes sense to provide a decent BigInt packer

casperisfine · 2022-02-15T08:34:52Z

Option renamed as oversized_integer_extension: true

tagomoris

Could you change to ... ?:

raise ArgumentError if oversized_integer_extension: true is specified with non-Integer class
not to require msgpack/bigint in default because its serialization format is not any agreed format - it should be required by users explicitly

tagomoris · 2022-02-15T13:33:46Z

ext/msgpack/packer.h

+        if(required_size > 8 && pk->has_bigint_ext_type) {
+            if(msgpack_packer_try_write_with_ext_type_lookup(pk, v)) {
+                return;
+            }


Does this code let msgpack_packer_write_u64 raise RangeError unless pk->has_bigint_ext_type, right?
If so, I want a comment to explain it here.

Yes, I wrote a spec for it, that's how I noticed JRuby was inconsistent.

Oh, sorry, I want a code comment at just after L482

casperisfine · 2022-02-15T14:00:02Z

@tagomoris it should be good now.

tagomoris

LGTM!

I've created a commit to minimize if-clause for older rubies as small as possible.
WDYT? 7f586b4

casperisfine · 2022-02-22T11:39:51Z

WDYT?

It cause two methods to be defined instead of one. Not the end of the world, but seems unnecessary. But if you really don't like def inside if, it's up to you.

tagomoris · 2022-02-22T11:57:43Z

Using define_method with Proc is alternative. Hmm.

casperisfine · 2022-02-22T11:59:06Z

I'd rather not, define_method create different ISeq that are slower than regular ones defined with def.

casperisfine · 2022-02-22T11:59:35Z

Is this only a stylistic issue? Because this pattern is fairly common.

casperisfine · 2022-02-22T12:00:20Z

define_method create different ISeq that are slower than regular ones defined with def.

You can search for the bmethod term (as in block methods)

tagomoris · 2022-02-22T12:07:00Z

I want to decrease points to confuse myself in the future. def in if makes me confused when I check code without memory of this change (for code about Ruby 2.6, will be EoL next month!)

casperisfine · 2022-02-22T12:08:37Z

Then maybe some extra commenting could help?

tagomoris · 2022-02-22T12:16:34Z

Could you write the implementation for the current Ruby versions at first (with appropriate code comments)?
Then, the compatibility follows.
I think it should decrease my confusion.

casperisfine · 2022-02-22T12:17:27Z

Of course.

Fix: msgpack#243 ```ruby factory = MessagePack::Factory.new factory.register_type( 0x42, Integer, packer: ->(int) { int.to_s }, unpacker: -> (payload) { Integer(payload) } ) factory.dump(42) # Use the native integer type factory.dump(2**400) # Use the registered packer ```

casperisfine · 2022-02-22T12:25:19Z

Done.

tagomoris

LGTM. Thanks!

casperisfine · 2022-02-22T12:55:36Z

Thanks for the merge!

Ref: msgpack/msgpack-ruby#244

casperisfine reviewed Jan 27, 2022

View changes

ext/msgpack/packer_ext_registry.h Outdated Show resolved Hide resolved

casperisfine mentioned this pull request Jan 27, 2022

Avoid creating singleton classes when looking up extensions #245

Merged

casperisfine force-pushed the max-big-num branch from da0246f to e56dd33 Compare Jan 27, 2022

casperisfine force-pushed the max-big-num branch 3 times, most recently from 7cdfe71 to 79e4fdc Compare Jan 28, 2022

casperisfine force-pushed the max-big-num branch from 79e4fdc to ff3dbe7 Compare Feb 8, 2022

casperisfine force-pushed the max-big-num branch from ff3dbe7 to d9d4a47 Compare Feb 14, 2022

casperisfine reviewed Feb 14, 2022

View changes

casperisfine force-pushed the max-big-num branch from d9d4a47 to bc498d3 Compare Feb 15, 2022

tagomoris requested changes Feb 15, 2022

View changes

tagomoris added the reviewing label Feb 15, 2022

casperisfine force-pushed the max-big-num branch from bc498d3 to 2f10ae2 Compare Feb 15, 2022

tagomoris added the new feature label Feb 16, 2022

tagomoris mentioned this pull request Feb 22, 2022

Make compat code simpler (smaller if-clause) #268

Closed

tagomoris approved these changes Feb 22, 2022

View changes

casperisfine force-pushed the max-big-num branch from 2f10ae2 to 84ca67c Compare Feb 22, 2022

tagomoris approved these changes Feb 22, 2022

View changes

tagomoris merged commit 9aa71da into msgpack:master Feb 22, 2022
23 checks passed

casperisfine pushed a commit to Shopify/paquito that referenced this issue Mar 8, 2022

Integrate Bigint support from msgpack master branch

1b349dc

Ref: msgpack/msgpack-ruby#244

casperisfine mentioned this pull request Mar 8, 2022

Integrate Bigint support from msgpack master branch Shopify/paquito#5

Merged

Apr	MAY	Jun
	03
2021	2022	2023

msgpack / msgpack-ruby Public

Support serializing bigint as extension types #244

Support serializing bigint as extension types #244

casperisfine commented Jan 27, 2022

tagomoris commented Jan 28, 2022

casperisfine commented Jan 28, 2022

tagomoris commented Feb 8, 2022

casperisfine commented Feb 8, 2022

casperisfine left a comment

casperisfine Feb 14, 2022

casperisfine Feb 14, 2022

casperisfine Feb 14, 2022

casperisfine commented Feb 15, 2022

tagomoris left a comment •

edited

tagomoris Feb 15, 2022

casperisfine Feb 15, 2022

tagomoris Feb 15, 2022

casperisfine commented Feb 15, 2022

tagomoris left a comment

casperisfine commented Feb 22, 2022

tagomoris commented Feb 22, 2022

casperisfine commented Feb 22, 2022

casperisfine commented Feb 22, 2022

casperisfine commented Feb 22, 2022

tagomoris commented Feb 22, 2022

casperisfine commented Feb 22, 2022

tagomoris commented Feb 22, 2022

casperisfine commented Feb 22, 2022

casperisfine commented Feb 22, 2022

tagomoris left a comment

casperisfine commented Feb 22, 2022

msgpack / msgpack-ruby Public

Support serializing bigint as extension types #244

Support serializing bigint as extension types #244

Conversation

casperisfine commented Jan 27, 2022

tagomoris commented Jan 28, 2022

casperisfine commented Jan 28, 2022

tagomoris commented Feb 8, 2022

casperisfine commented Feb 8, 2022

casperisfine left a comment

casperisfine Feb 14, 2022

Choose a reason for hiding this comment

casperisfine Feb 14, 2022

Choose a reason for hiding this comment

casperisfine Feb 14, 2022

Choose a reason for hiding this comment

casperisfine commented Feb 15, 2022

tagomoris left a comment • edited

tagomoris Feb 15, 2022

Choose a reason for hiding this comment

casperisfine Feb 15, 2022

Choose a reason for hiding this comment

tagomoris Feb 15, 2022

Choose a reason for hiding this comment

casperisfine commented Feb 15, 2022

tagomoris left a comment

casperisfine commented Feb 22, 2022

tagomoris commented Feb 22, 2022

casperisfine commented Feb 22, 2022

casperisfine commented Feb 22, 2022

casperisfine commented Feb 22, 2022

tagomoris commented Feb 22, 2022

casperisfine commented Feb 22, 2022

tagomoris commented Feb 22, 2022

casperisfine commented Feb 22, 2022

casperisfine commented Feb 22, 2022

tagomoris left a comment

casperisfine commented Feb 22, 2022

tagomoris left a comment •

edited