exec: fix NaN comparison logic

[solongordon]

I added special NaN handling for float comparisons. In SQL, NaNs are
treated as less than any other float value.

Thankfully I'm not seeing a performance hit when I run our sort
benchmarks with float64 values.

Fixes #38751

Release note: None

[cockroach-teamcity]

This change is 

[asubiotto]

it's great how clean of a change this is

[solongordon]

bors r+

[jordanlewis]

Mind blown that this doesn't affect benchmarks too much. Does it affect selection benchmarks?

[solongordon]

Yeah, good point, I should have run that one too. Noticeable drop-off in performance there unfortunately.

name                                                      old time/op    new time/op    delta
SelLTFloat64Float64ConstOp/useSel=true,hasNulls=true-4      1.28µs ± 1%    2.68µs ± 2%  +109.95%  (p=0.016 n=4+5)
SelLTFloat64Float64ConstOp/useSel=true,hasNulls=false-4      816ns ± 9%    1601ns ± 1%   +96.10%  (p=0.008 n=5+5)
SelLTFloat64Float64ConstOp/useSel=false,hasNulls=true-4     1.19µs ± 2%    1.82µs ± 3%   +52.50%  (p=0.008 n=5+5)
SelLTFloat64Float64ConstOp/useSel=false,hasNulls=false-4     645ns ± 3%    1877ns ± 1%  +191.04%  (p=0.008 n=5+5)
SelLTFloat64Float64Op/useSel=true,hasNulls=true-4           1.80µs ± 4%    2.41µs ± 2%   +33.47%  (p=0.008 n=5+5)
SelLTFloat64Float64Op/useSel=true,hasNulls=false-4           807ns ± 4%    1303ns ± 2%   +61.52%  (p=0.008 n=5+5)
SelLTFloat64Float64Op/useSel=false,hasNulls=true-4          1.38µs ±11%    2.76µs ± 2%   +99.86%  (p=0.008 n=5+5)
SelLTFloat64Float64Op/useSel=false,hasNulls=false-4          641ns ± 8%    1171ns ± 2%   +82.77%  (p=0.008 n=5+5)

name                                                      old speed      new speed      delta
SelLTFloat64Float64ConstOp/useSel=true,hasNulls=true-4    6.40GB/s ± 1%  3.05GB/s ± 2%   -52.35%  (p=0.016 n=4+5)
SelLTFloat64Float64ConstOp/useSel=true,hasNulls=false-4   10.1GB/s ± 9%   5.1GB/s ± 1%   -49.09%  (p=0.008 n=5+5)
SelLTFloat64Float64ConstOp/useSel=false,hasNulls=true-4   6.86GB/s ± 2%  4.50GB/s ± 3%   -34.41%  (p=0.008 n=5+5)
SelLTFloat64Float64ConstOp/useSel=false,hasNulls=false-4  12.7GB/s ± 2%   4.4GB/s ± 1%   -65.62%  (p=0.008 n=5+5)
SelLTFloat64Float64Op/useSel=true,hasNulls=true-4         9.08GB/s ± 4%  6.80GB/s ± 2%   -25.12%  (p=0.008 n=5+5)
SelLTFloat64Float64Op/useSel=true,hasNulls=false-4        20.3GB/s ± 4%  12.6GB/s ± 2%   -38.12%  (p=0.008 n=5+5)
SelLTFloat64Float64Op/useSel=false,hasNulls=true-4        11.9GB/s ±10%   5.9GB/s ± 2%   -50.14%  (p=0.008 n=5+5)
SelLTFloat64Float64Op/useSel=false,hasNulls=false-4       25.6GB/s ± 8%  14.0GB/s ± 2%   -45.34%  (p=0.008 n=5+5)

[jordanlewis]

I figured. I bet if we changed things around to do another pass over the vectors searching for NaNs, things would be different... but I think the effort there is probably too high to justify. You could also imagine doing something similar to nulls/sel array for NaNs, and fast path when you know there's no NaNs at all, but again I don't think it's worth it here.

[yuzefovich]

Hm, it appears that bors just ignored "r+" from several hours ago :)

[jordanlewis]

I wonder if we could avoid this second check by changing a == b to be int64(a) == int64(b) or whatever - aren't the bits of NaN the same?

[solongordon]

Good thought. I think we would use math.Float64bits.

[solongordon]

BTW, I was worried this might be slower than the normal float equality check, but it's actually faster! Presumably because the normal one has to do NaN checks.

[solongordon]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)