Skip to content

Commit

Permalink
Fix hash for double and real types when new nan is disabled
Browse files Browse the repository at this point in the history
The hash function wasn't gated by the use-new-nan-definition field,
which meant it returned different results for -0 (which in the new
definition is the same as +0, and in the old definition was not).

This also changes the method call for the real hash function with
use-new-nan-definition enabled, but the two functions have the same
implementation, so the change is purely for clarity/consistency.
  • Loading branch information
rschlussel committed Jun 24, 2024
1 parent 794cbae commit 5ed9da6
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,12 @@ public boolean equalTo(Block leftBlock, int leftPosition, Block rightBlock, int
@Override
public long hash(Block block, int position)
{
// convert to canonical NaN if necessary
return doubleHashCode(longBitsToDouble(block.getLong(position)));
Double doubleValue = longBitsToDouble(block.getLong(position));
if (!useNewNanDefintion) {
// convert to canonical NaN if necessary
return AbstractLongType.hash(doubleToLongBits(doubleValue));
}
return doubleHashCode(doubleValue);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static com.facebook.presto.common.type.TypeUtils.realCompare;
import static com.facebook.presto.common.type.TypeUtils.realEquals;
import static com.facebook.presto.common.type.TypeUtils.realHashCode;
import static java.lang.Float.floatToIntBits;
import static java.lang.Float.intBitsToFloat;
import static java.lang.Math.toIntExact;
import static java.lang.String.format;
Expand Down Expand Up @@ -64,7 +65,12 @@ public boolean equalTo(Block leftBlock, int leftPosition, Block rightBlock, int
@Override
public long hash(Block block, int position)
{
return realHashCode(intBitsToFloat(block.getInt(position)));
float value = intBitsToFloat(block.getInt(position));
if (!useNewNanDefinition) {
// convert to canonical NaN if necessary
return AbstractIntType.hash(floatToIntBits(value));
}
return realHashCode(value);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ public static long realHashCode(float value)
// canonicalize +0 and -0 to a single value
value = value == -0 ? 0 : value;
// floatToIntBits converts all NaNs to the same representation
return AbstractLongType.hash(floatToIntBits(value));
return AbstractIntType.hash(floatToIntBits(value));
}

public static int realCompare(float a, float b)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,12 @@
import org.testng.annotations.Test;

import static com.facebook.presto.common.type.DoubleType.DOUBLE;
import static com.facebook.presto.common.type.DoubleType.OLD_NAN_DOUBLE;
import static com.facebook.presto.common.type.RealType.OLD_NAN_REAL;
import static java.lang.Double.doubleToLongBits;
import static java.lang.Double.doubleToRawLongBits;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotEquals;

public class TestDoubleType
extends AbstractTestType
Expand Down Expand Up @@ -68,4 +71,22 @@ public void testNaNHash()
assertEquals(DOUBLE.hash(blockBuilder, 0), DOUBLE.hash(blockBuilder, 2));
assertEquals(DOUBLE.hash(blockBuilder, 0), DOUBLE.hash(blockBuilder, 3));
}

@Test
public void testLegacyDoubleHash()
{
BlockBuilder blockBuilder = new LongArrayBlockBuilder(null, 4);
blockBuilder.writeLong(doubleToLongBits(Double.parseDouble("-0")));
blockBuilder.writeLong(doubleToLongBits(Double.parseDouble("0")));
assertNotEquals(OLD_NAN_DOUBLE.hash(blockBuilder, 0), OLD_NAN_REAL.hash(blockBuilder, 1));
}

@Test
public void testDoubleHash()
{
BlockBuilder blockBuilder = new LongArrayBlockBuilder(null, 4);
blockBuilder.writeLong(doubleToLongBits(Double.parseDouble("-0")));
blockBuilder.writeLong(doubleToLongBits(Double.parseDouble("0")));
assertEquals(DOUBLE.hash(blockBuilder, 0), DOUBLE.hash(blockBuilder, 1));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@
import com.facebook.presto.common.block.IntArrayBlockBuilder;
import org.testng.annotations.Test;

import static com.facebook.presto.common.type.RealType.OLD_NAN_REAL;
import static com.facebook.presto.common.type.RealType.REAL;
import static java.lang.Float.floatToIntBits;
import static java.lang.Float.floatToRawIntBits;
import static java.lang.Float.intBitsToFloat;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotEquals;

public class TestRealType
extends AbstractTestType
Expand Down Expand Up @@ -71,4 +73,22 @@ public void testNaNHash()
assertEquals(REAL.hash(blockBuilder, 0), REAL.hash(blockBuilder, 2));
assertEquals(REAL.hash(blockBuilder, 0), REAL.hash(blockBuilder, 3));
}

@Test
public void testLegacyRealHash()
{
BlockBuilder blockBuilder = new IntArrayBlockBuilder(null, 4);
blockBuilder.writeInt(floatToIntBits(Float.parseFloat("-0")));
blockBuilder.writeInt(floatToIntBits(Float.parseFloat("0")));
assertNotEquals(OLD_NAN_REAL.hash(blockBuilder, 0), OLD_NAN_REAL.hash(blockBuilder, 1));
}

@Test
public void testRealHash()
{
BlockBuilder blockBuilder = new IntArrayBlockBuilder(null, 4);
blockBuilder.writeInt(floatToIntBits(Float.parseFloat("-0")));
blockBuilder.writeInt(floatToIntBits(Float.parseFloat("0")));
assertEquals(REAL.hash(blockBuilder, 0), REAL.hash(blockBuilder, 1));
}
}

0 comments on commit 5ed9da6

Please sign in to comment.