Number normalization for Crystal::Hasher #5276

akzhan · 2017-11-11T19:29:31Z

As declared by Crystal language reference, 1_i32.hash should be equal to 1_f64.hash.

This method must have the property that a == b implies a.hash == b.hash.

With this Pull request it's true.

Extracted from #4675, also replaces #4581.

Highly inspired by Python hashes, and has great optimizations for some cases by @funny-falcon.

akzhan · 2017-11-11T19:30:47Z

P.S.: price of hash normalization is minimal: i8-32 has no price, i64/128 has one div, floats need one frexp op (but float32/float64 use fast integer IEEE754 path). No price for others,

faustinoaq · 2017-11-11T22:50:25Z

src/crystal/hasher.cr

+
+  private HASH_BITS      = 61
+  private HASH_MODULUS   = (1_i64 << HASH_BITS) - 1
+  private HASH_NAN       =      0_u64


Is it a formatting issue?

No, it is aligned with _ of below 314159_u64. I think it is expected.

IMO it should should have single space after =.

akzhan · 2017-11-12T02:19:49Z

I have not touched/optimized BigDecimal class here because it was born too late (#4876). I hope that @vegai helps to optimize number normalization for these numbers too.

But generic float implementation will work anyway too.

vegai · 2017-11-12T14:01:57Z

@akzhan Roger, I'll try to do this soon.

As declared by Crystal language reference, 1i32.hash should equal to 1f64.hash. Extracted from crystal-lang#4675, also replaces crystal-lang#4581.

luislavena · 2017-11-13T15:42:25Z

spec/std/crystal/hasher_spec.cr

@@ -51,6 +52,15 @@ describe "Crystal::Hasher" do
      2.hash.should eq(2_u64.hash)
    end

+    it "Big i64 numbers should be hashed ok" do
+      Int64::MAX.hash.should eq (Int64::MAX.hash)


Please remove spaces between eq and the value that is expected to be equal to.

luislavena · 2017-11-13T15:42:47Z

spec/std/crystal/hasher_spec.cr

+    end
+
+    pending "128bit types should be hashed ok" do
+      1.to_i128.hash.should eq (1_i8.hash)


Same as above. Please remove spaces between eq and the arguments given to it.

luislavena · 2017-11-13T15:48:15Z

Hello @akzhan, Thank you for the PR!

I read the link you provided about Object#hash and doesn't state what you described in this request. Perhaps this was discussed in another issue by Crystal core developers? Can you provide the link to that instead?

Thank you.

akzhan · 2017-11-13T17:06:59Z

Hello @luislavena - Crystal language reference was my starting point.

There is definition:

This method must have the property that a == b implies a.hash == b.hash.

Current Crystal (any version) doesn't follow this definition (1_i32.hash != 1_f64.hash in master).

Nice point how it is in Python: https://github.com/python/cpython/blob/master/Python/pyhash.c#L1

P.S.: spacing fixed.

akzhan · 2017-11-15T22:31:59Z

Is it GTG?

RX14 · 2017-11-18T16:36:43Z

src/big/big_float.cr

+# :nodoc:
+struct Crystal::Hasher
+  def float(value : BigFloat)
+    permute(float_normalize_wrap(value) do |value|


Please assign the result of the float_normalize_wrap to a variable which is passed to permute. Having a multi-line block inside a method call arguments is just bad style.

@akzhan You missed that one ;)

@Sija Thanks!

RX14 · 2017-11-18T16:46:23Z

src/crystal/hasher.cr

+  end
+
+  def float(value : Float32)
+    permute(float_normalize_wrap(value) do |value|


RX14 · 2017-11-18T16:46:29Z

src/crystal/hasher.cr

+  end
+
+  def float(value : Float64)
+    permute(float_normalize_wrap(value) do |value|


RX14 · 2017-11-18T16:46:33Z

src/crystal/hasher.cr

+  end
+
+  def float(value : Float)
+    permute(float_normalize_wrap(value) do |value|


RX14 · 2017-11-18T16:47:19Z

Apart from that, I think this is gtg.

akzhan · 2017-11-18T16:56:55Z

Changes applied

akzhan · 2017-11-23T21:53:36Z

I know that few of team known this area, but please merge it. It's required to get Crystal stable in terms of production use.

may be @mverzilli ?

P.S.: It is part of Hashing #4675.

luislavena

All my comments have been addressed. 👍

mverzilli · 2017-11-24T17:31:58Z

Thanks @akzhan!

Conditionally run this on 64bits platforms. Ref #5276

Conditionally run this on 64bits platforms. Ref crystal-lang#5276

akzhan changed the title ~~Indtruces real Number normalization for Crystal::Hasher~~ Intrudces real Number normalization for Crystal::Hasher Nov 11, 2017

akzhan changed the title ~~Intrudces real Number normalization for Crystal::Hasher~~ Introduces real Number normalization for Crystal::Hasher Nov 11, 2017

akzhan force-pushed the number_normalization branch 2 times, most recently from 43c6611 to 4dfa15a Compare November 11, 2017 20:31

faustinoaq reviewed Nov 11, 2017

View reviewed changes

akzhan added 2 commits November 12, 2017 17:31

Introduces real Number normalization for Crystal::Hasher.

a276993

As declared by Crystal language reference, 1i32.hash should equal to 1f64.hash. Extracted from crystal-lang#4675, also replaces crystal-lang#4581.

hash specializations for BigInt, BigFloat, BigRational.

c46503e

akzhan force-pushed the number_normalization branch from a6fc65f to c46503e Compare November 12, 2017 14:31

akzhan changed the title ~~Introduces real Number normalization for Crystal::Hasher~~ Number normalization for Crystal::Hasher Nov 12, 2017

akzhan mentioned this pull request Nov 12, 2017

Implement BigDecimal #4876

Merged

luislavena suggested changes Nov 13, 2017

View reviewed changes

follow @luislavena

46b2eef

RX14 requested changes Nov 18, 2017

View reviewed changes

Followed @RX14

252539c

normalized_hash

c92aa17

RX14 approved these changes Nov 18, 2017

View reviewed changes

RX14 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib labels Nov 18, 2017

oops @Sija :)

d6d2952

luislavena approved these changes Nov 24, 2017

View reviewed changes

mverzilli approved these changes Nov 24, 2017

View reviewed changes

mverzilli merged commit 3f06d20 into crystal-lang:master Nov 24, 2017

mverzilli added this to the Next milestone Nov 24, 2017

akzhan deleted the number_normalization branch November 24, 2017 17:48

akzhan mentioned this pull request Nov 30, 2017

Equal floats and ints have different hashes #2818

Closed

luislavena mentioned this pull request Dec 12, 2017

Execute 128bits Hasher specs that are now passing #5380

Merged

bcardiff pushed a commit that referenced this pull request Jun 5, 2018

Execute 128bits Hasher specs that are now passing (#5380)

de3c95e

Conditionally run this on 64bits platforms. Ref #5276

chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018

Execute 128bits Hasher specs that are now passing (crystal-lang#5380)

07ff287

Conditionally run this on 64bits platforms. Ref crystal-lang#5276

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Number normalization for Crystal::Hasher #5276

Number normalization for Crystal::Hasher #5276

akzhan commented Nov 11, 2017 •

edited

Loading

akzhan commented Nov 11, 2017 •

edited

Loading

faustinoaq Nov 11, 2017

makenowjust Nov 11, 2017

Sija Nov 12, 2017

akzhan commented Nov 12, 2017 •

edited

Loading

vegai commented Nov 12, 2017

luislavena Nov 13, 2017

luislavena Nov 13, 2017

luislavena commented Nov 13, 2017

akzhan commented Nov 13, 2017 •

edited

Loading

akzhan commented Nov 15, 2017

RX14 Nov 18, 2017

Sija Nov 18, 2017

akzhan Nov 18, 2017

RX14 Nov 18, 2017

RX14 Nov 18, 2017

RX14 Nov 18, 2017

RX14 commented Nov 18, 2017

akzhan commented Nov 18, 2017

akzhan commented Nov 23, 2017 •

edited

Loading

luislavena left a comment

mverzilli commented Nov 24, 2017

Number normalization for Crystal::Hasher #5276

Number normalization for Crystal::Hasher #5276

Conversation

akzhan commented Nov 11, 2017 • edited Loading

akzhan commented Nov 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akzhan commented Nov 12, 2017 • edited Loading

vegai commented Nov 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luislavena commented Nov 13, 2017

akzhan commented Nov 13, 2017 • edited Loading

akzhan commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Nov 18, 2017

akzhan commented Nov 18, 2017

akzhan commented Nov 23, 2017 • edited Loading

luislavena left a comment

Choose a reason for hiding this comment

mverzilli commented Nov 24, 2017

akzhan commented Nov 11, 2017 •

edited

Loading

akzhan commented Nov 11, 2017 •

edited

Loading

akzhan commented Nov 12, 2017 •

edited

Loading

akzhan commented Nov 13, 2017 •

edited

Loading

akzhan commented Nov 23, 2017 •

edited

Loading