Copilot commented on code in PR #143:
URL: https://github.com/apache/paimon-cpp/pull/143#discussion_r3542023395
##########
src/paimon/common/executor/future.h:
##########
@@ -31,27 +31,42 @@
namespace paimon {
-/// Submits a function to be executed asynchronously on a given executor and
returns a future.
+/// Submits a function to be executed asynchronously on a given executor and
returns a
+/// future.
///
-/// This function wraps the provided callable and submits it to the provided
executor. The function
-/// captures the result using a std::promise, which is used to fulfill the
returned std::future. If
-/// the callable throws an exception, the exception is captured and set in the
promise.
+/// This function wraps the provided callable (`func`) and submits it to the
provided `executor`.
+/// The function captures the result of `func` using a `std::promise`, which
is used to fulfill
+/// the returned `std::future`. If the callable throws an exception, the
exception is captured
+/// and set in the `promise`.
+///
+/// @tparam Func The type of the callable function.
+/// @param executor The executor to run the function on. Must provide an `Add`
method for task
+/// submission.
+/// @param func The function to execute asynchronously. Can be any callable
object.
+/// @return std::future<decltype(func())> A future that holds the result of
the function
+/// execution.
+///
+/// @note If `func` returns `void`, the returned future is of type
`std::future<void>`.
template <typename Func>
-auto Via(Executor* executor, Func&& func) ->
std::future<std::invoke_result_t<Func>> {
- using ResultType = std::invoke_result_t<Func>;
+auto Via(Executor* executor, Func&& func) -> std::future<decltype(func())> {
+ using ResultType = decltype(func());
+ // Check if func is callable (invocable)
static_assert(std::is_invocable_v<Func>, "func must be callable");
Review Comment:
The invocability check and return type deduction are based on `Func` /
`decltype(func())`, but `func` is a named variable (an lvalue) and the lambda
later also calls it as an lvalue. This can accept callables that are only
invocable on rvalues (passing the `static_assert`) and then fail to compile
when invoked. Consider basing traits on `Func&` (e.g., invocable and result
type) to match the actual call site semantics.
##########
src/paimon/common/executor/future.h:
##########
@@ -31,27 +31,42 @@
namespace paimon {
-/// Submits a function to be executed asynchronously on a given executor and
returns a future.
+/// Submits a function to be executed asynchronously on a given executor and
returns a
+/// future.
///
-/// This function wraps the provided callable and submits it to the provided
executor. The function
-/// captures the result using a std::promise, which is used to fulfill the
returned std::future. If
-/// the callable throws an exception, the exception is captured and set in the
promise.
+/// This function wraps the provided callable (`func`) and submits it to the
provided `executor`.
+/// The function captures the result of `func` using a `std::promise`, which
is used to fulfill
+/// the returned `std::future`. If the callable throws an exception, the
exception is captured
+/// and set in the `promise`.
+///
+/// @tparam Func The type of the callable function.
+/// @param executor The executor to run the function on. Must provide an `Add`
method for task
+/// submission.
+/// @param func The function to execute asynchronously. Can be any callable
object.
+/// @return std::future<decltype(func())> A future that holds the result of
the function
+/// execution.
+///
+/// @note If `func` returns `void`, the returned future is of type
`std::future<void>`.
template <typename Func>
-auto Via(Executor* executor, Func&& func) ->
std::future<std::invoke_result_t<Func>> {
- using ResultType = std::invoke_result_t<Func>;
+auto Via(Executor* executor, Func&& func) -> std::future<decltype(func())> {
+ using ResultType = decltype(func());
+ // Check if func is callable (invocable)
static_assert(std::is_invocable_v<Func>, "func must be callable");
+ // Check if func is an empty std::function
if constexpr (std::is_constructible_v<std::function<void()>, Func>) {
std::function<void()> test_func = func;
if (!test_func) {
Review Comment:
Constructing a `std::function` just to detect emptiness can allocate and
adds runtime overhead to every `Via` call for compatible callables. A more
targeted check (e.g., only when `Func` is actually a `std::function<...>` or a
function pointer) would avoid the extra allocation/copy while still catching
the 'empty std::function' case.
##########
src/paimon/core/casting/cast_executor_test.cpp:
##########
@@ -165,8 +165,23 @@ class CastExecutorTest : public ::testing::Test {
for (size_t i = 0; i < src_data.size(); i++) {
ASSERT_OK_AND_ASSIGN(Literal result,
cast_executor->Cast(src_literals[i], target_type));
ASSERT_FALSE(result.IsNull());
- ASSERT_EQ(target_literals[i], result)
- << target_literals[i].ToString() << "->" << result.ToString();
+ if (target_literals[i] != result) {
Review Comment:
This fallback will fail for NaN cases: `target_literals[i] != result` is
true for NaN, and `ASSERT_NEAR(NaN, NaN, ...)` fails. If NaN conversions are
expected/possible, handle them explicitly (e.g., treat both-NaN as equal)
before calling `ASSERT_NEAR`.
##########
src/paimon/core/casting/cast_executor_test.cpp:
##########
@@ -165,8 +165,23 @@ class CastExecutorTest : public ::testing::Test {
for (size_t i = 0; i < src_data.size(); i++) {
ASSERT_OK_AND_ASSIGN(Literal result,
cast_executor->Cast(src_literals[i], target_type));
ASSERT_FALSE(result.IsNull());
- ASSERT_EQ(target_literals[i], result)
- << target_literals[i].ToString() << "->" << result.ToString();
+ if (target_literals[i] != result) {
+ // Fall back to approximate comparison for floating-point
types, because
+ // different conversion paths (e.g. decimal->float vs float
literal) may
+ // produce slightly different bit representations.
+ if (target_field_type == FieldType::FLOAT) {
+ ASSERT_NEAR(target_literals[i].GetValue<float>(),
result.GetValue<float>(),
+ 1E-5)
+ << target_literals[i].ToString() << "->" <<
result.ToString();
+ } else if (target_field_type == FieldType::DOUBLE) {
+ ASSERT_NEAR(target_literals[i].GetValue<double>(),
result.GetValue<double>(),
+ 1E-5)
+ << target_literals[i].ToString() << "->" <<
result.ToString();
Review Comment:
This fallback will fail for NaN cases: `target_literals[i] != result` is
true for NaN, and `ASSERT_NEAR(NaN, NaN, ...)` fails. If NaN conversions are
expected/possible, handle them explicitly (e.g., treat both-NaN as equal)
before calling `ASSERT_NEAR`.
##########
src/paimon/common/sst/sst_file_io_test.cpp:
##########
@@ -173,7 +173,7 @@ TEST_P(SstFileIOTest, TestSimple) {
ASSERT_EQ("looooooooooong-值-15", string15);
}
-TEST_P(SstFileIOTest, TestJavaCompatibility) {
+TEST_P(SstFileIOTest, DISABLED_TestJavaCompatibility) {
Review Comment:
Disabling this test without an inline reason makes it harder to track
whether it should be re-enabled. Please add a brief comment explaining why it’s
disabled (e.g., missing fixtures, flakiness) and ideally reference an
issue/task ID for follow-up.
--
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]
