lidavidm commented on code in PR #47017:
URL: https://github.com/apache/arrow/pull/47017#discussion_r2229838032
##########
cpp/src/arrow/flight/sql/odbc/odbcabstraction/calendar_utils.cc:
##########
@@ -39,13 +41,30 @@ int64_t GetTodayTimeFromEpoch() {
#endif
}
-void GetTimeForSecondsSinceEpoch(tm& date, int64_t value) {
-#if defined(_WIN32)
- gmtime_s(&date, &value);
-#else
- time_t time_value = static_cast<time_t>(value);
- gmtime_r(&time_value, &date);
-#endif
+void GetTimeForSecondsSinceEpoch(std::tm& out_tm, int64_t seconds_since_epoch)
{
+ try {
+ std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>
timepoint{
+ std::chrono::seconds{seconds_since_epoch}};
+
+ std::chrono::year_month_day ymd =
std::chrono::floor<std::chrono::days>(timepoint);
+
+ std::chrono::hh_mm_ss<std::chrono::seconds> timeofday{
+ timepoint - std::chrono::floor<std::chrono::days>(timepoint)};
+
+ std::memset(&out_tm, 0, sizeof(std::tm));
+
+ out_tm.tm_year = static_cast<int>(ymd.year()) - 1900;
+ out_tm.tm_mon = static_cast<unsigned>(ymd.month()) - 1;
Review Comment:
`tm_mon` appears to be `int`, so can we use `int` instead of `unsigned` here?
##########
cpp/src/arrow/flight/sql/odbc/odbcabstraction/calendar_utils.cc:
##########
@@ -39,13 +41,30 @@ int64_t GetTodayTimeFromEpoch() {
#endif
}
-void GetTimeForSecondsSinceEpoch(tm& date, int64_t value) {
-#if defined(_WIN32)
- gmtime_s(&date, &value);
-#else
- time_t time_value = static_cast<time_t>(value);
- gmtime_r(&time_value, &date);
-#endif
+void GetTimeForSecondsSinceEpoch(std::tm& out_tm, int64_t seconds_since_epoch)
{
+ try {
+ std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>
timepoint{
+ std::chrono::seconds{seconds_since_epoch}};
+
+ std::chrono::year_month_day ymd =
std::chrono::floor<std::chrono::days>(timepoint);
+
+ std::chrono::hh_mm_ss<std::chrono::seconds> timeofday{
+ timepoint - std::chrono::floor<std::chrono::days>(timepoint)};
+
+ std::memset(&out_tm, 0, sizeof(std::tm));
+
+ out_tm.tm_year = static_cast<int>(ymd.year()) - 1900;
+ out_tm.tm_mon = static_cast<unsigned>(ymd.month()) - 1;
+ out_tm.tm_mday = static_cast<unsigned>(ymd.day());
Review Comment:
Ditto for `tm_mday`
##########
cpp/src/arrow/flight/sql/odbc/odbcabstraction/calendar_utils.cc:
##########
@@ -39,13 +41,30 @@ int64_t GetTodayTimeFromEpoch() {
#endif
}
-void GetTimeForSecondsSinceEpoch(tm& date, int64_t value) {
-#if defined(_WIN32)
- gmtime_s(&date, &value);
-#else
- time_t time_value = static_cast<time_t>(value);
- gmtime_r(&time_value, &date);
-#endif
+void GetTimeForSecondsSinceEpoch(std::tm& out_tm, int64_t seconds_since_epoch)
{
+ try {
+ std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>
timepoint{
+ std::chrono::seconds{seconds_since_epoch}};
+
+ std::chrono::year_month_day ymd =
std::chrono::floor<std::chrono::days>(timepoint);
+
+ std::chrono::hh_mm_ss<std::chrono::seconds> timeofday{
+ timepoint - std::chrono::floor<std::chrono::days>(timepoint)};
Review Comment:
Why is this not `timepoint - ymd`?
(Also, is there a need to use initializer syntax here or can it be
`timeofday = timepoint - ymd`?)
##########
cpp/src/arrow/flight/sql/odbc/odbcabstraction/calendar_utils.cc:
##########
@@ -39,13 +41,30 @@ int64_t GetTodayTimeFromEpoch() {
#endif
}
-void GetTimeForSecondsSinceEpoch(tm& date, int64_t value) {
-#if defined(_WIN32)
- gmtime_s(&date, &value);
-#else
- time_t time_value = static_cast<time_t>(value);
- gmtime_r(&time_value, &date);
-#endif
+void GetTimeForSecondsSinceEpoch(std::tm& out_tm, int64_t seconds_since_epoch)
{
+ try {
+ std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>
timepoint{
+ std::chrono::seconds{seconds_since_epoch}};
+
+ std::chrono::year_month_day ymd =
std::chrono::floor<std::chrono::days>(timepoint);
+
+ std::chrono::hh_mm_ss<std::chrono::seconds> timeofday{
+ timepoint - std::chrono::floor<std::chrono::days>(timepoint)};
+
+ std::memset(&out_tm, 0, sizeof(std::tm));
+
+ out_tm.tm_year = static_cast<int>(ymd.year()) - 1900;
+ out_tm.tm_mon = static_cast<unsigned>(ymd.month()) - 1;
+ out_tm.tm_mday = static_cast<unsigned>(ymd.day());
+ out_tm.tm_hour = static_cast<int>(timeofday.hours().count());
+ out_tm.tm_min = static_cast<int>(timeofday.minutes().count());
+ out_tm.tm_sec = static_cast<int>(timeofday.seconds().count());
+ } catch (const std::exception&) {
Review Comment:
...Actually, why are we catching an exception in the first place? I don't
think anything here throws?
##########
cpp/src/arrow/flight/sql/odbc/odbcabstraction/calendar_utils.cc:
##########
@@ -39,13 +41,30 @@ int64_t GetTodayTimeFromEpoch() {
#endif
}
-void GetTimeForSecondsSinceEpoch(tm& date, int64_t value) {
-#if defined(_WIN32)
- gmtime_s(&date, &value);
-#else
- time_t time_value = static_cast<time_t>(value);
- gmtime_r(&time_value, &date);
-#endif
+void GetTimeForSecondsSinceEpoch(std::tm& out_tm, int64_t seconds_since_epoch)
{
+ try {
+ std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>
timepoint{
+ std::chrono::seconds{seconds_since_epoch}};
+
+ std::chrono::year_month_day ymd =
std::chrono::floor<std::chrono::days>(timepoint);
+
+ std::chrono::hh_mm_ss<std::chrono::seconds> timeofday{
+ timepoint - std::chrono::floor<std::chrono::days>(timepoint)};
+
+ std::memset(&out_tm, 0, sizeof(std::tm));
+
+ out_tm.tm_year = static_cast<int>(ymd.year()) - 1900;
+ out_tm.tm_mon = static_cast<unsigned>(ymd.month()) - 1;
+ out_tm.tm_mday = static_cast<unsigned>(ymd.day());
+ out_tm.tm_hour = static_cast<int>(timeofday.hours().count());
+ out_tm.tm_min = static_cast<int>(timeofday.minutes().count());
+ out_tm.tm_sec = static_cast<int>(timeofday.seconds().count());
+ } catch (const std::exception&) {
Review Comment:
@alinaliBQ I'm not sure we want to ignore exceptions here. Ideally this
function would be `Result<std::tm> GetTimeForSecondsSinceEpoch(int64_t)` (or
`Status ...` with an out parameter if there's a reason to do that. Also note
that conventionally out parameters go at the end:
https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs)
##########
cpp/src/arrow/flight/sql/odbc/flight_sql/accessors/timestamp_array_accessor.cc:
##########
@@ -44,25 +44,22 @@ int64_t GetConversionToSecondsDivisor(TimeUnit::type unit) {
return divisor;
}
-uint32_t CalculateFraction(TimeUnit::type unit, uint64_t units_since_epoch) {
- // Convert the given remainder and time unit to nanoseconds
- // since the fraction field on TIMESTAMP_STRUCT is in nanoseconds.
Review Comment:
Could we keep the doc comment? (Ideally as an actual doc comment)
##########
cpp/src/arrow/flight/sql/odbc/odbcabstraction/calendar_utils.cc:
##########
@@ -39,13 +41,30 @@ int64_t GetTodayTimeFromEpoch() {
#endif
}
-void GetTimeForSecondsSinceEpoch(tm& date, int64_t value) {
-#if defined(_WIN32)
- gmtime_s(&date, &value);
-#else
- time_t time_value = static_cast<time_t>(value);
- gmtime_r(&time_value, &date);
-#endif
+void GetTimeForSecondsSinceEpoch(std::tm& out_tm, int64_t seconds_since_epoch)
{
+ try {
+ std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>
timepoint{
+ std::chrono::seconds{seconds_since_epoch}};
+
+ std::chrono::year_month_day ymd =
std::chrono::floor<std::chrono::days>(timepoint);
+
+ std::chrono::hh_mm_ss<std::chrono::seconds> timeofday{
+ timepoint - std::chrono::floor<std::chrono::days>(timepoint)};
+
+ std::memset(&out_tm, 0, sizeof(std::tm));
+
+ out_tm.tm_year = static_cast<int>(ymd.year()) - 1900;
+ out_tm.tm_mon = static_cast<unsigned>(ymd.month()) - 1;
+ out_tm.tm_mday = static_cast<unsigned>(ymd.day());
+ out_tm.tm_hour = static_cast<int>(timeofday.hours().count());
+ out_tm.tm_min = static_cast<int>(timeofday.minutes().count());
+ out_tm.tm_sec = static_cast<int>(timeofday.seconds().count());
+ } catch (const std::exception&) {
+ std::memset(&out_tm, 0, sizeof(tm));
Review Comment:
Just memset once up front at the start of the function
##########
cpp/src/arrow/flight/sql/odbc/odbcabstraction/calendar_utils.cc:
##########
@@ -39,13 +41,30 @@ int64_t GetTodayTimeFromEpoch() {
#endif
}
-void GetTimeForSecondsSinceEpoch(tm& date, int64_t value) {
-#if defined(_WIN32)
- gmtime_s(&date, &value);
-#else
- time_t time_value = static_cast<time_t>(value);
- gmtime_r(&time_value, &date);
-#endif
+void GetTimeForSecondsSinceEpoch(std::tm& out_tm, int64_t seconds_since_epoch)
{
+ try {
+ std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>
timepoint{
+ std::chrono::seconds{seconds_since_epoch}};
+
+ std::chrono::year_month_day ymd =
std::chrono::floor<std::chrono::days>(timepoint);
+
+ std::chrono::hh_mm_ss<std::chrono::seconds> timeofday{
+ timepoint - std::chrono::floor<std::chrono::days>(timepoint)};
+
+ std::memset(&out_tm, 0, sizeof(std::tm));
+
+ out_tm.tm_year = static_cast<int>(ymd.year()) - 1900;
+ out_tm.tm_mon = static_cast<unsigned>(ymd.month()) - 1;
+ out_tm.tm_mday = static_cast<unsigned>(ymd.day());
+ out_tm.tm_hour = static_cast<int>(timeofday.hours().count());
+ out_tm.tm_min = static_cast<int>(timeofday.minutes().count());
+ out_tm.tm_sec = static_cast<int>(timeofday.seconds().count());
+ } catch (const std::exception&) {
Review Comment:
However that is unrelated to the issue at hand so we can follow up in a new
issue
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
