Copilot commented on code in PR #1023:
URL: https://github.com/apache/iceberg-cpp/pull/1023#discussion_r4281138385
##########
src/iceberg/util/bucket_util.cc:
##########
@@ -20,18 +20,41 @@
#include "iceberg/util/bucket_util.h"
#include <limits>
+#include <optional>
#include <utility>
#include "iceberg/exception.h"
#include "iceberg/expression/literal.h"
-#include "iceberg/util/bucket_util_internal.h"
#include "iceberg/util/endian.h"
#include "iceberg/util/murmurhash3_internal.h"
#include "iceberg/util/temporal_util.h"
namespace iceberg {
namespace {
+
+/// MurmurHash3_x86_32 takes a signed 32-bit length. Iceberg's bucket
transform is
+/// defined on byte sequences of at most 2^31 - 1 bytes (Java arrays are
int-indexed),
+/// and longer inputs would wrap the length negative and make the hash loop
read far
+/// outside the buffer, so they are rejected instead.
+constexpr size_t kMaxHashBytes =
static_cast<size_t>(std::numeric_limits<int32_t>::max());
+
+/// \brief Returns the number of bytes `HashLiteral` will hash for `literal`.
+///
+/// Returns `std::nullopt` for values hashed as fixed-width integers, which
have no
+/// length limit.
+std::optional<size_t> HashedByteSize(const Literal& literal) {
+ switch (literal.type()->type_id()) {
+ case TypeId::kString:
+ return std::get<std::string>(literal.value()).size();
+ case TypeId::kBinary:
+ case TypeId::kFixed:
+ return std::get<std::vector<uint8_t>>(literal.value()).size();
+ default:
+ return std::nullopt;
+ }
+}
+
Review Comment:
`HashedByteSize()` duplicates knowledge about which `TypeId`s are
variable-length and hashed as raw bytes. This creates a drift risk if hashing
behavior changes (e.g., a new variable-length type is added or an existing
mapping changes) but this helper is not updated, reintroducing the possibility
of an oversize input reaching `HashBytes`. A more maintainable approach is to
perform the size check inline in `BucketIndex` in the same switch that decides
how the value is hashed (handle `kString/kBinary/kFixed` explicitly there), so
the guarded types cannot diverge from the hashing dispatch.
##########
src/iceberg/util/bucket_util.cc:
##########
@@ -113,29 +136,16 @@ int32_t HashLiteral<TypeId::kFixed>(const Literal&
literal) {
} // namespace
int32_t BucketUtils::HashBytes(std::span<const uint8_t> bytes) {
- return internal::HashBytes(bytes.data(), bytes.size());
-}
-
-namespace internal {
-
-int32_t HashBytes(const uint8_t* data, size_t len) {
- // MurmurHash3_x86_32 takes a signed 32-bit length. Passing a larger size_t
- // would wrap it negative and make the hash loop read far outside `data`
- // (and silently hash the wrong prefix for even larger inputs). Iceberg's
- // bucket transform is defined on byte sequences of at most 2^31 - 1 bytes
- // (Java arrays), so reject anything longer before narrowing.
ICEBERG_CHECK_OR_DIE(
- len <= static_cast<size_t>(std::numeric_limits<int32_t>::max()),
+ bytes.size() <= kMaxHashBytes,
"BucketUtils::HashBytes: input length {} exceeds the maximum supported "
"length {}",
- len, std::numeric_limits<int32_t>::max());
+ bytes.size(), kMaxHashBytes);
Review Comment:
`ICEBERG_CHECK_OR_DIE` commonly implies process termination rather than an
exception; however, the PR description indicates `BucketUtils::HashBytes`
should throw `IcebergError` for oversized inputs. If `ICEBERG_CHECK_OR_DIE` can
abort in some build modes, this would be a reliability issue for direct
callers. Consider using an error path that unconditionally throws
`IcebergError` (or otherwise matches the documented behavior) so oversized
inputs don’t become a fatal error outside `BucketIndex`.
##########
src/iceberg/util/bucket_util.cc:
##########
@@ -113,29 +136,16 @@ int32_t HashLiteral<TypeId::kFixed>(const Literal&
literal) {
} // namespace
int32_t BucketUtils::HashBytes(std::span<const uint8_t> bytes) {
- return internal::HashBytes(bytes.data(), bytes.size());
-}
-
-namespace internal {
-
-int32_t HashBytes(const uint8_t* data, size_t len) {
- // MurmurHash3_x86_32 takes a signed 32-bit length. Passing a larger size_t
- // would wrap it negative and make the hash loop read far outside `data`
- // (and silently hash the wrong prefix for even larger inputs). Iceberg's
- // bucket transform is defined on byte sequences of at most 2^31 - 1 bytes
- // (Java arrays), so reject anything longer before narrowing.
ICEBERG_CHECK_OR_DIE(
- len <= static_cast<size_t>(std::numeric_limits<int32_t>::max()),
+ bytes.size() <= kMaxHashBytes,
"BucketUtils::HashBytes: input length {} exceeds the maximum supported "
"length {}",
- len, std::numeric_limits<int32_t>::max());
+ bytes.size(), kMaxHashBytes);
int32_t hash_value = 0;
- MurmurHash3_x86_32(data, static_cast<int>(len), 0, &hash_value);
+ MurmurHash3_x86_32(bytes.data(), static_cast<int>(bytes.size()), 0,
&hash_value);
Review Comment:
The guard bounds `bytes.size()` by `INT32_MAX` (`kMaxHashBytes`), but the
call narrows to `int` (`static_cast<int>(bytes.size())`). On platforms where
`int` is narrower than 32 bits, this can still overflow and pass a
negative/incorrect length into `MurmurHash3_x86_32`. Consider guarding against
`std::numeric_limits<int>::max()` as well (e.g., use `min(kMaxHashBytes,
static_cast<size_t>(std::numeric_limits<int>::max()))`) or add a
`static_assert(std::numeric_limits<int>::max() >=
std::numeric_limits<int32_t>::max())` to make the assumption explicit.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
