GH-50517: [C++] RunEndEncodedBuilder does not merge consecutive NaN values

[andishgar]

Rationale for this change

RunEndEncodedBuilder does not merge consecutive NaN values into a single run.

What changes are included in this PR?

Enable merging consecutive NaN values into a single run.

Are these changes tested?

Yes, I added a unit test covering consecutive NaN values.

Are there any user-facing changes?

No.

• GitHub Issue: [C++] RunEndEncodedBuilder does not merge consecutive NaN values #50517

[pitrou]

What happens if you append different NaNs values? For example quiet_NaN vs. signaling_NaN.

[andishgar]

@pitrou, the current run-end logic considers both quiet and signaling NaNs equal. Should we add an option to distinguish between them?

[pitrou]

@pitrou, the current run-end logic considers both quiet and signaling NaNs equal. Should we add an option to distinguish between them?

I'm not sure, but I think NaNs should just be compared by physical value, so that different NaNs can be preserved when run-end-encoding. @HuaHuaY What do you think?

[HuaHuaY]

I checked arrow-rs. Its behavior seems inconsistent with ours. It uses != to compare NaN values, so it does not merge any NaNs, even if their physical values ​​are identical. It doesn't have a method like AppendScalar; based on the example in the issue, it seems to get 6 run-ends. https://github.com/apache/arrow-rs/blob/0a5979560ac67135134c3618519b9b7384edf4d2/arrow-array/src/builder/primitive_run_builder.rs#L176

NaNs should just be compared by physical value

If we want to allow the merging of NaN values, I agree with your point. I think we should compare the physical values.

cpp/src/arrow/compute/kernels/vector_run_end_encode.cc:597 treats HALF_FLOAT/FLOAT/DOUBLE as UINT16/UINT32/UINT64.

cpp/src/arrow/compute/kernels/vector_run_end_encode.cc:307 refuses to merge NaNs of type HALF_FLOAT/FLOAT/DOUBLE within nested types.

While checking Arrow's code, I discovered another issue. When constructing dictionary encoded arrays for Parquet in #50807, I compare the physical values ​​of NaNs, but Arrow's dictionary builder does not do this (see cpp/src/arrow/util/hashing.h:140). Perhaps we could open a separate PR to fix that.

[andishgar]

@HuaHuaY @pitrou there seems to be a paradox in the run-end encode compute kernel.

The comment below mentions that NaN values are not merged for nested types:

arrow/cpp/src/arrow/compute/kernels/vector_run_end_encode.cc

Lines 307 to 308 in 5c2ff72

	// Avoid merging floating-point values whose representations may differ. Signed
	// zeros compare unequal, and NaNs remain in separate runs to preserve payloads.

For nested types, the following code shows that consecutive NaN values are not merged:

TEST(MyTest, Struct) {
  auto struct_type = struct_({field("a", float32())});
  auto array = ArrayFromJSON(struct_type, R"([{"a":NaN},{"a":NaN}])");
  Datum datum(array);
  auto result =
      RunEndEncode(datum).ValueOrDie().array_as<RunEndEncodedArray>();
  ARROW_LOGGER_INFO("", result->ToString());
}

The result is:

-- run_ends:
  [
    1,
    2
  ]
-- values:
  -- is_valid: all not null
  -- child 0 type: float
    [
      nan,
      nan
    ]

However, for non-nested types, consecutive NaN values are merged:

TEST(MyTest, Float) {
  std::shared_ptr<Array> array;
  ArrayFromVector<FloatType>({NAN, NAN}, &array);
  Datum datum(array);
  auto result =
      RunEndEncode(datum).ValueOrDie().array_as<RunEndEncodedArray>();
  ARROW_LOGGER_INFO("", result->ToString());
}

The result is:

-- run_ends:
  [
    2
  ]
-- values:
  [
    nan
  ]

So it seems that the behavior differs between nested and non-nested types, despite the comment suggesting that NaN values should not be merged for nested types. Is this the expected behavior?

[HuaHuaY]

So it seems that the behavior differs between nested and non-nested types, despite the comment suggesting that NaN values should not be merged for nested types. Is this the expected behavior?

The code at cpp/src/arrow/compute/kernels/vector_run_end_encode.cc:597, which I mentioned earlier, determines the behavior for non-nested types.

I am not familiar with Arrow Compute. But I don't think there are any correctness issues with either behavior; both allow the original data to be recovered, with the difference being encoding efficiency. If we want to merge NaNs with identical physical representations, perhaps we could open a separate PR to do that for nested types.

[andishgar]

@pitrou, I could not find anything in the spec that says whether NaNs should be merged. I suggest that we should add options for both the compute kernel and the RunEndEncoded builder to control whether NaNs, signed zeros, atol, ulp_distance_, and identical physical values should be considered equal and merged. What do you think?

[pitrou]

I suggest that we should add options for both the compute kernel and the RunEndEncoded builder to control whether NaNs, signed zeros, atol, ulp_distance_, and identical physical values should be considered equal and merged. What do you think?

That sounds overkill to me, especially as NaNs are rare in practice.

The only constraint here is that run-end-encoding must be entirely lossless: decoding must reproduce the original array. So we should only merge identical values, not values that are close to each other.

[andishgar]

@pitrou

As the result of this discussion is that float values should be merged only when they are identical in their physical representation, I suggest adding another flag to arrow::EqualOptions to compare float values based on their bit pattern in a separate PR, then applying this comparison to the REE compute kernel for nested types in another PR, and to the REE builder here.

What is your opinion?

[pitrou]

As the result of this discussion is that float values should be merged only when they are identical in their physical representation, I suggest adding another flag to arrow::EqualOptions to compare float values based on their bit pattern in a separate PR, then applying this comparison to the REE compute kernel for nested types in another PR, and to the REE builder here.

I'm not sure. I think we can instead special-case all fixed-width types for the purpose of comparing REE values. This can also make the REE builder faster than allocating a new Scalar for every logical value.

Something like this in ree_util.h (untested, just a sketch):

struct PhysicalValue {
  friend bool operator==(const PhysicalValue& left, const PhysicalValue& right) {
    return ...;
  }

  static PhysicalValueForComparison LookupArray(const Array& array, int64_t index) {
    if (array.IsNull(index)) {
      return {std::shared_ptr<Scalar>{}};
    }
    auto typed_lookup = [&](auto* concrete_type) {
       using ArrowType = std::decay_t<decltype(*concrete_type)>;
       if constexpr(is_fixed_width(ArrowType::id) && !is_boolean(ArrowType::id)) {
         const int64_t byte_width = concrete_type->byte_width();
         const ArrayData& data = *array.data();
         return {data.buffers[1]->span_as<uint8_t>.subspan(
             (index + data.offset) * byte_width, byte_width)};
       } else {
         return array.GetScalar(index);
       }
    };
    return VisitType(*array.type(), typed_lookup);
  }

  // Three possible states:
  // - A span of bytes for a non-null non-boolean primitive value
  // - A nullptr Scalar for a null value
  // - A non-null generic Scalar otherwise
  std::variant<std::shared_ptr<Scalar>, std::span<const uint8_t>> payload_;
};