diff --git a/cpp/src/arrow/compute/expression_test.cc b/cpp/src/arrow/compute/expression_test.cc index 10c1b348e62..7d7aa04855d 100644 --- a/cpp/src/arrow/compute/expression_test.cc +++ b/cpp/src/arrow/compute/expression_test.cc @@ -2178,6 +2178,20 @@ TEST(Expression, SimplifyThenExecute) { AssertDatumsEqual(evaluated, simplified_evaluated, /*verbose=*/true); } +TEST(Expression, SimplifyIsInSignedZero) { + // Parquet statistics of a column holding only zeros are min=-0.0 and max=+0.0, which + // become the guarantee `x == -0.0`. It must not rule out `x in {0.0}`. + SetLookupOptions in_zero_f64{ArrayFromJSON(float64(), "[0.0]")}; + Simplify{call("is_in", {field_ref("f64")}, in_zero_f64)} + .WithGuarantee(equal(field_ref("f64"), literal(-0.0))) + .Expect(true); + + SetLookupOptions in_zero_f32{ArrayFromJSON(float32(), "[0.0]")}; + Simplify{call("is_in", {field_ref("f32")}, in_zero_f32)} + .WithGuarantee(equal(field_ref("f32"), literal(-0.0f))) + .Expect(true); +} + TEST(Expression, SimplifyIsInThenExecute) { auto input = RecordBatchFromJSON(kBoringSchema, R"([ {"i64": 2, "i32": 5}, diff --git a/cpp/src/arrow/compute/kernels/scalar_set_lookup.cc b/cpp/src/arrow/compute/kernels/scalar_set_lookup.cc index c6a3562233c..1eb30a8b168 100644 --- a/cpp/src/arrow/compute/kernels/scalar_set_lookup.cc +++ b/cpp/src/arrow/compute/kernels/scalar_set_lookup.cc @@ -15,6 +15,8 @@ // specific language governing permissions and limitations // under the License. +#include + #include "arrow/array/array_base.h" #include "arrow/compute/api_scalar.h" #include "arrow/compute/cast.h" @@ -22,6 +24,7 @@ #include "arrow/compute/kernels/util_internal.h" #include "arrow/compute/registry_internal.h" #include "arrow/type.h" +#include "arrow/type_traits.h" #include "arrow/util/bit_util.h" #include "arrow/util/bitmap_writer.h" #include "arrow/util/hashing.h" @@ -47,6 +50,7 @@ struct SetLookupState : public SetLookupStateBase { Status Init(const SetLookupOptions& options) { this->null_matching_behavior = options.GetNullMatchingBehavior(); + this->value_set_is_floating = is_floating(options.value_set.type()->id()); if (options.value_set.is_array()) { const ArrayData& value_set = *options.value_set.array(); memo_index_to_value_index.reserve(value_set.length); @@ -92,8 +96,9 @@ struct SetLookupState : public SetLookupStateBase { DCHECK_EQ(memo_index, memo_size); memo_index_to_value_index.push_back(index); }; - RETURN_NOT_OK(lookup_table->GetOrInsert( - v, std::move(on_found), std::move(on_not_found), &unused_memo_index)); + RETURN_NOT_OK(lookup_table->GetOrInsert(LookupKey(v), std::move(on_found), + std::move(on_not_found), + &unused_memo_index)); ++index; return Status::OK(); }; @@ -114,9 +119,22 @@ struct SetLookupState : public SetLookupStateBase { return VisitArraySpanInline(data, visit_valid, visit_null); } + // Floating-point values are looked up through their unsigned integer bit patterns + // (see InitStateVisitor), which would make -0.0 and +0.0 different keys. Fold -0.0 + // into +0.0 so that set membership agrees with `equal`. + template + T LookupKey(T value) const { + if constexpr (std::is_integral_v && !std::is_same_v) { + constexpr T kNegativeZero = static_cast(T{1} << (sizeof(T) * 8 - 1)); + if (value == kNegativeZero && value_set_is_floating) return T{0}; + } + return value; + } + using MemoTable = typename HashTraits::MemoTableType; std::optional lookup_table; // use optional for delayed initialization MemoryPool* memory_pool; + bool value_set_is_floating = false; // When there are duplicates in value_set, the MemoTable indices must // be mapped back to indices in the value_set. std::vector memo_index_to_value_index; @@ -305,7 +323,7 @@ struct IndexInVisitor { VisitArraySpanInline( input, [&](T v) { - int32_t index = state.lookup_table->Get(v); + int32_t index = state.lookup_table->Get(state.LookupKey(v)); if (index != -1) { bitmap_writer.Set(); @@ -441,7 +459,7 @@ struct IsInVisitor { VisitArraySpanInline( input, [&](T v) { - if (state.lookup_table->Get(v) != -1) { // true + if (state.lookup_table->Get(state.LookupKey(v)) != -1) { // true writer_boolean.Set(); writer_null.Set(); } else if (state.null_matching_behavior == SetLookupOptions::INCONCLUSIVE && diff --git a/cpp/src/arrow/compute/kernels/scalar_set_lookup_test.cc b/cpp/src/arrow/compute/kernels/scalar_set_lookup_test.cc index dbd89489920..3fa6905f47f 100644 --- a/cpp/src/arrow/compute/kernels/scalar_set_lookup_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_set_lookup_test.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -36,6 +37,7 @@ #include "arrow/compute/kernels/test_util_internal.h" #include "arrow/result.h" #include "arrow/status.h" +#include "arrow/testing/builder.h" #include "arrow/testing/gtest_compat.h" #include "arrow/testing/gtest_util.h" #include "arrow/type.h" @@ -305,6 +307,48 @@ TYPED_TEST(TestIsInKernelPrimitive, IsIn) { CheckIsIn(type, "[]", "[]", "[]"); } +// -0.0 and +0.0 compare equal, so each of them must be found in a set holding the other +template +void CheckIsInSignedZero() { + using CType = typename ArrowType::c_type; + std::shared_ptr input, positive_zero, negative_zero, both; + ArrayFromVector({CType(-0.0), CType(0.0), CType(1.5)}, &input); + ArrayFromVector({CType(0.0)}, &positive_zero); + ArrayFromVector({CType(-0.0)}, &negative_zero); + ArrayFromVector({CType(2.5), CType(-0.0), CType(0.0)}, &both); + + CheckIsIn(input, positive_zero, "[true, true, false]"); + CheckIsIn(input, negative_zero, "[true, true, false]"); + CheckIsIn(input, both, "[true, true, false]"); + + std::shared_ptr other; + ArrayFromVector({CType(2.5)}, &other); + CheckIsInChunked(std::make_shared(ArrayVector{input}), + std::make_shared(ArrayVector{other, positive_zero}), + ChunkedArrayFromJSON(boolean(), {"[true, true, false]"})); +} + +// The bit pattern of -0.0 is the smallest signed integer of the same width, +// which must not be folded into 0 +template +void CheckIsInSmallestInteger() { + using CType = typename ArrowType::c_type; + std::shared_ptr input, zero, smallest; + ArrayFromVector({std::numeric_limits::min(), CType(0)}, &input); + ArrayFromVector({CType(0)}, &zero); + ArrayFromVector({std::numeric_limits::min()}, &smallest); + + CheckIsIn(input, zero, "[false, true]"); + CheckIsIn(input, smallest, "[true, false]"); +} + +TEST_F(TestIsInKernel, SignedZero) { + CheckIsInSignedZero(); + CheckIsInSignedZero(); + CheckIsInSmallestInteger(); + CheckIsInSmallestInteger(); +} + TEST_F(TestIsInKernel, NullType) { auto type = null(); @@ -1430,6 +1474,40 @@ TEST_F(TestIndexInKernel, Boolean) { CheckIndexIn(boolean(), "[null, null, null, null]", "[null]", "[0, 0, 0, 0]"); } +template +void CheckIndexInSignedZero(TestIndexInKernel* test) { + using CType = typename ArrowType::c_type; + std::shared_ptr input, positive_zero, negative_zero, both; + ArrayFromVector({CType(-0.0), CType(0.0), CType(1.5)}, &input); + ArrayFromVector({CType(0.0)}, &positive_zero); + ArrayFromVector({CType(-0.0)}, &negative_zero); + ArrayFromVector({CType(2.5), CType(-0.0), CType(0.0)}, &both); + + test->CheckIndexIn(input, positive_zero, "[0, 0, null]"); + test->CheckIndexIn(input, negative_zero, "[0, 0, null]"); + // Both zeros are the same value: the first one in the value set is reported + test->CheckIndexIn(input, both, "[1, 1, null]"); +} + +template +void CheckIndexInSmallestInteger(TestIndexInKernel* test) { + using CType = typename ArrowType::c_type; + std::shared_ptr input, zero, smallest; + ArrayFromVector({std::numeric_limits::min(), CType(0)}, &input); + ArrayFromVector({CType(0)}, &zero); + ArrayFromVector({std::numeric_limits::min()}, &smallest); + + test->CheckIndexIn(input, zero, "[null, 0]"); + test->CheckIndexIn(input, smallest, "[0, null]"); +} + +TEST_F(TestIndexInKernel, SignedZero) { + CheckIndexInSignedZero(this); + CheckIndexInSignedZero(this); + CheckIndexInSmallestInteger(this); + CheckIndexInSmallestInteger(this); +} + TEST_F(TestIndexInKernel, ImplicitlyCastValueSet) { auto input = ArrayFromJSON(int8(), "[0, 1, 2, 3, 4, 5, 6, 7, 8]"); diff --git a/python/pyarrow/tests/test_compute.py b/python/pyarrow/tests/test_compute.py index d4d44e9347e..a8f71997fd6 100644 --- a/python/pyarrow/tests/test_compute.py +++ b/python/pyarrow/tests/test_compute.py @@ -3462,6 +3462,15 @@ def test_index_in(): assert result.to_pylist() == [0, None, None, 0, None, 1] +@pytest.mark.parametrize("typ", [pa.float32(), pa.float64()]) +def test_is_in_index_in_signed_zero(typ): + # -0.0 == 0.0, so membership must not depend on the sign of the zero + arr = pa.array([-0.0, 0.0, 1.5], type=typ) + for value_set in (pa.array([0.0], type=typ), pa.array([-0.0], type=typ)): + assert pc.is_in(arr, value_set=value_set).to_pylist() == [True, True, False] + assert pc.index_in(arr, value_set=value_set).to_pylist() == [0, 0, None] + + def test_quantile(): arr = pa.array([1, 2, 3, 4]) diff --git a/python/pyarrow/tests/test_dataset.py b/python/pyarrow/tests/test_dataset.py index 9ddfdfdc9c4..a0f55304889 100644 --- a/python/pyarrow/tests/test_dataset.py +++ b/python/pyarrow/tests/test_dataset.py @@ -3155,6 +3155,18 @@ def test_filter_equal_null(tempdir, dataset_reader): assert table.num_rows == 0 +@pytest.mark.parquet +def test_filter_isin_signed_zero(tempdir, dataset_reader): + # A column holding only zeros has Parquet statistics min=-0.0 and max=0.0, + # which must not make the fragment look like it cannot contain 0.0 + table = pa.table({"x": [0.0, 0.0]}) + _, path = _create_single_file(tempdir, table) + dataset = ds.dataset(str(path)) + + filter_ = ds.field("x").isin([0.0]) + assert dataset_reader.to_table(dataset, filter=filter_).equals(table) + + @pytest.mark.parquet def test_filter_compute_expression(tempdir, dataset_reader): table = pa.table({