Copilot commented on code in PR #266:
URL: https://github.com/apache/fluss-rust/pull/266#discussion_r2776722986
##########
bindings/cpp/src/lib.rs:
##########
@@ -618,6 +622,12 @@ impl Table {
self.table_info.clone(),
);
+ let all_columns = self.table_info.get_schema().columns();
+ let projected_columns: Vec<_> = column_indices
+ .iter()
+ .map(|&i| all_columns[i].clone())
+ .collect();
Review Comment:
`projected_columns` is built via direct indexing (`all_columns[i]`), which
will panic on an out-of-range projection index coming from the C++ API. Because
this runs inside the FFI boundary, a panic will abort the process rather than
returning a clean error. Please validate indices (e.g., use `get()` and return
a `Result` error) before building `projected_columns`.
```suggestion
.map(|&i| {
all_columns
.get(i)
.cloned()
.ok_or_else(|| {
format!(
"Invalid column index {i}: schema has {}
columns",
all_columns.len()
)
})
})
.collect::<Result<_, String>>()?;
```
##########
bindings/cpp/src/table.cpp:
##########
@@ -22,12 +22,45 @@
#include "ffi_converter.hpp"
#include "rust/cxx.h"
#include <arrow/c/bridge.h>
+#include <ctime>
// todo: bindings/cpp/BUILD.bazel still doesn’t declare Arrow include/link
dependencies.
// In environments where Bazel does not already have Arrow available, this
will fail at compile/link time.
#include <arrow/record_batch.h>
namespace fluss {
+static constexpr int kSecondsPerDay = 24 * 60 * 60;
+
+Date Date::FromYMD(int year, int month, int day) {
+ std::tm tm{};
+ tm.tm_year = year - 1900;
+ tm.tm_mon = month - 1;
+ tm.tm_mday = day;
+ std::time_t epoch_seconds = timegm(&tm);
+ return {static_cast<int32_t>(epoch_seconds / kSecondsPerDay)};
+}
+
+int Date::Year() const {
+ std::time_t epoch_seconds = static_cast<std::time_t>(days_since_epoch) *
kSecondsPerDay;
+ std::tm tm{};
+ gmtime_r(&epoch_seconds, &tm);
+ return tm.tm_year + 1900;
+}
+
+int Date::Month() const {
+ std::time_t epoch_seconds = static_cast<std::time_t>(days_since_epoch) *
kSecondsPerDay;
+ std::tm tm{};
+ gmtime_r(&epoch_seconds, &tm);
+ return tm.tm_mon + 1;
+}
+
+int Date::Day() const {
+ std::time_t epoch_seconds = static_cast<std::time_t>(days_since_epoch) *
kSecondsPerDay;
+ std::tm tm{};
+ gmtime_r(&epoch_seconds, &tm);
Review Comment:
`timegm` and `gmtime_r` are non-standard/POSIX APIs and are not available on
all platforms (notably Windows) and may require feature macros on some libc
implementations. This can make the C++ bindings fail to compile outside POSIX
environments. Consider replacing with a portable YMD<->days-since-epoch
conversion (pure arithmetic), or provide platform-specific implementations
(e.g., `_mkgmtime`/`gmtime_s` on Windows and `timegm`/`gmtime_r` on POSIX)
behind `#ifdef`s.
```suggestion
// Portable wrappers for converting between UTC calendar dates and epoch
seconds.
static std::time_t timegm_utc(std::tm* tm) {
#if defined(_WIN32)
// _mkgmtime interprets the tm structure as UTC and returns time_t
seconds since epoch.
return _mkgmtime(tm);
#else
return ::timegm(tm);
#endif
}
static std::tm gmtime_utc(std::time_t epoch_seconds) {
std::tm tm{};
#if defined(_WIN32)
// gmtime_s has (tm*, time_t*) parameter order on Windows.
gmtime_s(&tm, &epoch_seconds);
#else
::gmtime_r(&epoch_seconds, &tm);
#endif
return tm;
}
Date Date::FromYMD(int year, int month, int day) {
std::tm tm{};
tm.tm_year = year - 1900;
tm.tm_mon = month - 1;
tm.tm_mday = day;
std::time_t epoch_seconds = timegm_utc(&tm);
return {static_cast<int32_t>(epoch_seconds / kSecondsPerDay)};
}
int Date::Year() const {
std::time_t epoch_seconds = static_cast<std::time_t>(days_since_epoch) *
kSecondsPerDay;
std::tm tm = gmtime_utc(epoch_seconds);
return tm.tm_year + 1900;
}
int Date::Month() const {
std::time_t epoch_seconds = static_cast<std::time_t>(days_since_epoch) *
kSecondsPerDay;
std::tm tm = gmtime_utc(epoch_seconds);
return tm.tm_mon + 1;
}
int Date::Day() const {
std::time_t epoch_seconds = static_cast<std::time_t>(days_since_epoch) *
kSecondsPerDay;
std::tm tm = gmtime_utc(epoch_seconds);
```
##########
bindings/cpp/src/lib.rs:
##########
@@ -665,6 +677,12 @@ impl Table {
self.table_info.clone(),
);
+ let all_columns = self.table_info.get_schema().columns();
+ let projected_columns: Vec<_> = column_indices
+ .iter()
+ .map(|&i| all_columns[i].clone())
+ .collect();
Review Comment:
Same issue as above: `projected_columns` is constructed with
`all_columns[i]` before calling `.project(...)`, so invalid indices can
panic/abort across FFI. Please bounds-check `column_indices` and return an
error instead of panicking.
--
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]
