wgtmac commented on code in PR #48345: URL: https://github.com/apache/arrow/pull/48345#discussion_r3986897851
########## cpp/src/arrow/util/alp/alp_constants_internal.h: ########## @@ -0,0 +1,288 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// Constants and type traits for ALP (Adaptive Lossless floating-Point) compression. +// Spec: https://github.com/apache/parquet-format/blob/master/Encodings.md#alp + +#pragma once + +#include <cstdint> + +#include "arrow/util/logging.h" + +namespace arrow::util::alp { + +// ---------------------------------------------------------------------- +// AlpConstants + +/// \brief Constants for Adaptive Lossless floating-Point (ALP) compression +/// See: https://github.com/apache/parquet-format/blob/master/Encodings.md#alp +class AlpConstants { + public: + /// Default number of elements compressed together as a unit. + /// The format supports arbitrary power-of-2 sizes via log_vector_size in the + /// page header (up to 2^kMaxLogVectorSize). + static constexpr int64_t kAlpVectorSize = 1024; + + /// Minimum supported log_vector_size value, i.e. a vector size of 8. + /// Mandated by the format spec (Encodings.md, "Must be in the inclusive + /// range [3, 15]"). + static constexpr uint8_t kMinLogVectorSize = 3; + + /// Maximum supported log_vector_size value. Capped at 15 because per-vector + /// element counts are stored as uint16_t (max 65535), and 2^16 = 65536 + /// would overflow. The cap allows vector sizes up to 32768. + static constexpr uint8_t kMaxLogVectorSize = 15; + + /// Sampling constants below are from the ALP paper (Afroozeh et al., + /// "ALP: Adaptive Lossless floating-Point Compression", SIGMOD 2023). + + /// Number of elements to use when determining sampling parameters. + static constexpr int64_t kSamplerVectorSize = 4096; + + /// Total number of elements in a rowgroup for sampling purposes. + /// 122880 = kSamplerVectorSize * 30 rowgroup vectors. + static constexpr int64_t kSamplerRowgroupSize = 122880; + + /// Number of samples to collect per vector during the sampling phase. Review Comment: A boarder comment on this file is that in essence most these constants can be moved to source files that use them. ########## cpp/src/arrow/util/alp/alp_test.cc: ########## @@ -0,0 +1,2063 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <cmath> +#include <cstdint> +#include <cstring> +#include <limits> +#include <random> +#include <string> +#include <vector> + +#include <gtest/gtest.h> + +#include "arrow/testing/gtest_util.h" +#include "arrow/util/alp/alp_codec_internal.h" +#include "arrow/util/alp/alp_constants_internal.h" +#include "arrow/util/alp/alp_internal.h" +#include "arrow/util/alp/alp_sampler_internal.h" +#include "arrow/util/bit_stream_utils_internal.h" +#include "arrow/util/bit_util.h" +#include "arrow/util/bpacking_internal.h" +#include "arrow/util/endian.h" +#include "arrow/util/ubsan.h" + +namespace arrow::util::alp { + +// ============================================================================ +// Test helpers +// ============================================================================ + +// Compares two floating-point ranges by bit pattern, not by operator==. +// ALP is a lossless codec, so its tests must verify bit-exact recovery: +// `0.0 == -0.0` (different bits) and `NaN != NaN` (identical bits) make +// `EXPECT_THAT(out, ElementsAreArray(in))` the wrong check here. On +// mismatch the failure message names the index and prints both the +// value and the underlying hex bits. +template <typename T> +::testing::AssertionResult IsBitwiseEqual(const std::vector<T>& actual, + const std::vector<T>& expected) { + static_assert(std::is_floating_point<T>::value, + "IsBitwiseEqual is for float/double only"); + using Bits = typename std::conditional<sizeof(T) == 4, uint32_t, uint64_t>::type; + if (actual.size() != expected.size()) { + return ::testing::AssertionFailure() << "size mismatch: actual=" << actual.size() + << " expected=" << expected.size(); + } + for (size_t i = 0; i < actual.size(); ++i) { + Bits a_bits = 0, e_bits = 0; + std::memcpy(&a_bits, &actual[i], sizeof(T)); + std::memcpy(&e_bits, &expected[i], sizeof(T)); + if (a_bits != e_bits) { + return ::testing::AssertionFailure() + << "bit-mismatch at index " << i << ": actual=" << actual[i] << " (bits 0x" + << std::hex << a_bits << "), expected=" << std::dec << expected[i] + << " (bits 0x" << std::hex << e_bits << ")"; + } + } + return ::testing::AssertionSuccess(); +} + +// ============================================================================ +// ALP Constants Tests +// ============================================================================ + +TEST(AlpConstantsTest, SamplerConstants) { Review Comment: I think test case like this is meaningless. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
