Skip to content

Commit

Permalink
Fix for DX-86729 & DX-90059 (partial testing)
Browse files Browse the repository at this point in the history 
DX-86729 (Windows-only) fix applied to all platforms for behaviour consistency

Merged in initial failing milliseconds test cases; should pass on the fix branch.

+Handle inconsistent implementation-defined negative mod

+Fixed negative epoch time division; should break broken tests now

+Probably actually fix negative timestamp division rounding issue

+Fix rest(?) of timestamp test suite

+Fix structure member names in test code

Change-Id: I16e4fe9befbff2327608bca92216a670492c8e44
  • Loading branch information
Paul Nienaber committed Apr 19, 2024
1 parent 156ac3d commit e59ac76
Show file tree
Hide file tree
Showing 3 changed files with 95 additions and 64 deletions.
18 changes: 16 additions & 2 deletions flight_sql/accessors/timestamp_array_accessor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#include "timestamp_array_accessor.h"
#include "odbcabstraction/calendar_utils.h"

#include <cmath>

using namespace arrow;

namespace {
Expand All @@ -32,23 +34,32 @@ int64_t GetConversionToSecondsDivisor(TimeUnit::type unit) {
return divisor;
}

uint32_t CalculateFraction(TimeUnit::type unit, uint64_t units_since_epoch) {
uint32_t CalculateFraction(TimeUnit::type unit, int64_t units_since_epoch) {
// Convert the given remainder and time unit to nanoseconds
// since the fraction field on TIMESTAMP_STRUCT is in nanoseconds.
switch (unit) {
case TimeUnit::SECOND:
return 0;
case TimeUnit::MILLI:
// 1000000 nanoseconds = 1 millisecond.
if (units_since_epoch < 0)
units_since_epoch += driver::odbcabstraction::MILLI_TO_SECONDS_DIVISOR *
(1 + std::abs(units_since_epoch / driver::odbcabstraction::MILLI_TO_SECONDS_DIVISOR));
return (units_since_epoch %
driver::odbcabstraction::MILLI_TO_SECONDS_DIVISOR) *
1000000;
case TimeUnit::MICRO:
// 1000 nanoseconds = 1 microsecond.
if (units_since_epoch < 0)
units_since_epoch += driver::odbcabstraction::MICRO_TO_SECONDS_DIVISOR *
(1 + std::abs(units_since_epoch / driver::odbcabstraction::MICRO_TO_SECONDS_DIVISOR));
return (units_since_epoch %
driver::odbcabstraction::MICRO_TO_SECONDS_DIVISOR) * 1000;
case TimeUnit::NANO:
// 1000 nanoseconds = 1 microsecond.
if (units_since_epoch < 0)
units_since_epoch += driver::odbcabstraction::MILLI_TO_SECONDS_DIVISOR *
(1 + std::abs(units_since_epoch / driver::odbcabstraction::MICRO_TO_SECONDS_DIVISOR));
return (units_since_epoch %
driver::odbcabstraction::NANO_TO_SECONDS_DIVISOR);
}
Expand Down Expand Up @@ -76,7 +87,10 @@ TimestampArrayFlightSqlAccessor<TARGET_TYPE, UNIT>::MoveSingleCell_impl(

int64_t value = this->GetArray()->Value(arrow_row);
const auto divisor = GetConversionToSecondsDivisor(UNIT);
const auto converted_result_seconds = value / divisor;
const auto converted_result_seconds =
(value < 0)
? ((value - (divisor - 1)) / divisor)
: value / divisor;
tm timestamp = {0};

GetTimeForSecondsSinceEpoch(timestamp, converted_result_seconds);
Expand Down
131 changes: 76 additions & 55 deletions flight_sql/accessors/timestamp_array_accessor_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,23 @@ using namespace arrow;
using namespace odbcabstraction;

TEST(TEST_TIMESTAMP, TIMESTAMP_WITH_MILLI) {
std::vector<int64_t> values = {86400370, 172800000, 259200000, 1649793238110LL,
345600000, 432000000, 518400000};

std::vector<int64_t> values = {86400370, 172800000, 259200000, 1649793238110LL, 345600000,
432000000, 518400000, -86399000, -86399999, -86399001};
std::vector<TIMESTAMP_STRUCT> expected = {
/* year(16), month(u16), day(u16), hour(u16), minute(u16), second(u16), fraction(u32) */
{1970, 1, 2, 0, 0, 0, 370000000},
{1970, 1, 3, 0, 0, 0, 0},
{1970, 1, 4, 0, 0, 0, 0},
{2022, 4, 12, 19, 53, 58, 110000000},
{1970, 1, 5, 0, 0, 0, 0},
{1970, 1, 6, 0, 0, 0, 0},
{1970, 1, 7, 0, 0, 0, 0},
{1969, 12, 31, 0, 0, 1, 0},
/* Tests both ends of the fraction rounding range to ensure we don't tip the wrong way */
{1969, 12, 31, 0, 0, 0, 1000000},
{1969, 12, 31, 0, 0, 0, 999000000},
};

std::shared_ptr<Array> timestamp_array;

auto timestamp_field = field("timestamp_field", timestamp(TimeUnit::MILLI));
Expand All @@ -40,26 +54,29 @@ TEST(TEST_TIMESTAMP, TIMESTAMP_WITH_MILLI) {
for (size_t i = 0; i < values.size(); ++i) {
ASSERT_EQ(sizeof(TIMESTAMP_STRUCT), strlen_buffer[i]);

tm date{};

auto converted_time = values[i] / MILLI_TO_SECONDS_DIVISOR;
GetTimeForSecondsSinceEpoch(date, converted_time);

ASSERT_EQ(buffer[i].year, 1900 + (date.tm_year));
ASSERT_EQ(buffer[i].month, date.tm_mon + 1);
ASSERT_EQ(buffer[i].day, date.tm_mday);
ASSERT_EQ(buffer[i].hour, date.tm_hour);
ASSERT_EQ(buffer[i].minute, date.tm_min);
ASSERT_EQ(buffer[i].second, date.tm_sec);

constexpr uint32_t NANOSECONDS_PER_MILLI = 1000000;
ASSERT_EQ(buffer[i].fraction, (values[i] % MILLI_TO_SECONDS_DIVISOR) * NANOSECONDS_PER_MILLI);
ASSERT_EQ(buffer[i].year, 1900 + (expected[i].year));
ASSERT_EQ(buffer[i].month, expected[i].month + 1);
ASSERT_EQ(buffer[i].day, expected[i].day);
ASSERT_EQ(buffer[i].hour, expected[i].hour);
ASSERT_EQ(buffer[i].minute, expected[i].minute);
ASSERT_EQ(buffer[i].second, expected[i].second);
ASSERT_EQ(buffer[i].fraction, expected[i].fraction);
}
}

TEST(TEST_TIMESTAMP, TIMESTAMP_WITH_SECONDS) {
std::vector<int64_t> values = {86400, 172800, 259200, 1649793238,
345600, 432000, 518400};
345600, 432000, 518400, -86399};
std::vector<TIMESTAMP_STRUCT> expected = {
/* year(16), month(u16), day(u16), hour(u16), minute(u16), second(u16), fraction(u32) */
{1970, 1, 2, 0, 0, 0, 0},
{1970, 1, 3, 0, 0, 0, 0},
{1970, 1, 4, 0, 0, 0, 0},
{2022, 4, 12, 19, 53, 58, 0},
{1970, 1, 5, 0, 0, 0, 0},
{1970, 1, 6, 0, 0, 0, 0},
{1970, 1, 7, 0, 0, 0, 0},
};

std::shared_ptr<Array> timestamp_array;

Expand All @@ -81,23 +98,26 @@ TEST(TEST_TIMESTAMP, TIMESTAMP_WITH_SECONDS) {

for (size_t i = 0; i < values.size(); ++i) {
ASSERT_EQ(sizeof(TIMESTAMP_STRUCT), strlen_buffer[i]);
tm date{};

auto converted_time = values[i];
GetTimeForSecondsSinceEpoch(date, converted_time);

ASSERT_EQ(buffer[i].year, 1900 + (date.tm_year));
ASSERT_EQ(buffer[i].month, date.tm_mon + 1);
ASSERT_EQ(buffer[i].day, date.tm_mday);
ASSERT_EQ(buffer[i].hour, date.tm_hour);
ASSERT_EQ(buffer[i].minute, date.tm_min);
ASSERT_EQ(buffer[i].second, date.tm_sec);
ASSERT_EQ(buffer[i].year, 1900 + (expected[i].year));
ASSERT_EQ(buffer[i].month, expected[i].month + 1);
ASSERT_EQ(buffer[i].day, expected[i].day);
ASSERT_EQ(buffer[i].hour, expected[i].hour);
ASSERT_EQ(buffer[i].minute, expected[i].minute);
ASSERT_EQ(buffer[i].second, expected[i].second);
ASSERT_EQ(buffer[i].fraction, 0);
}
}

TEST(TEST_TIMESTAMP, TIMESTAMP_WITH_MICRO) {
std::vector<int64_t> values = {86400000000, 1649793238000000};
std::vector<int64_t> values = {86400000000, 1649793238000000, -86399999999, -86399000001};
std::vector<TIMESTAMP_STRUCT> expected = {
/* year(16), month(u16), day(u16), hour(u16), minute(u16), second(u16), fraction(u32) */
{1970, 1, 2, 0, 0, 0, 0},
{2022, 4, 12, 19, 53, 58, 0},
{1969, 12, 31, 0, 0, 0, 1000},
{1969, 12, 31, 0, 0, 0, 999999000},
};

std::shared_ptr<Array> timestamp_array;

Expand All @@ -120,24 +140,28 @@ TEST(TEST_TIMESTAMP, TIMESTAMP_WITH_MICRO) {
for (size_t i = 0; i < values.size(); ++i) {
ASSERT_EQ(sizeof(TIMESTAMP_STRUCT), strlen_buffer[i]);

tm date{};

auto converted_time = values[i] / MICRO_TO_SECONDS_DIVISOR;
GetTimeForSecondsSinceEpoch(date, converted_time);

ASSERT_EQ(buffer[i].year, 1900 + (date.tm_year));
ASSERT_EQ(buffer[i].month, date.tm_mon + 1);
ASSERT_EQ(buffer[i].day, date.tm_mday);
ASSERT_EQ(buffer[i].hour, date.tm_hour);
ASSERT_EQ(buffer[i].minute, date.tm_min);
ASSERT_EQ(buffer[i].second, date.tm_sec);
constexpr uint32_t MICROS_PER_NANO = 1000;
ASSERT_EQ(buffer[i].fraction, (values[i] % MICRO_TO_SECONDS_DIVISOR) * MICROS_PER_NANO);
ASSERT_EQ(buffer[i].year, 1900 + (expected[i].year));
ASSERT_EQ(buffer[i].month, expected[i].month + 1);
ASSERT_EQ(buffer[i].day, expected[i].day);
ASSERT_EQ(buffer[i].hour, expected[i].hour);
ASSERT_EQ(buffer[i].minute, expected[i].minute);
ASSERT_EQ(buffer[i].second, expected[i].second);
ASSERT_EQ(buffer[i].fraction, expected[i].fraction);
}
}

TEST(TEST_TIMESTAMP, TIMESTAMP_WITH_NANO) {
std::vector<int64_t> values = {86400000010000, 1649793238000000000};
std::vector<int64_t> values = {86400000010000, 1649793238000000000, -86399999999999, -86399000000001
86400000000001, 86400999999999};
std::vector<TIMESTAMP_STRUCT> expected = {
/* year(16), month(u16), day(u16), hour(u16), minute(u16), second(u16), fraction(u32) */
{1970, 1, 2, 0, 0, 0, 10000},
{2022, 4, 12, 19, 53, 58, 0},
{1969, 12, 31, 0, 0, 0, 1},
{1969, 12, 31, 0, 0, 0, 999999999},
{1970, 1, 2, 0, 0, 0, 1},
{1970, 1, 2, 0, 0, 0, 999999999},
};

std::shared_ptr<Array> timestamp_array;

Expand All @@ -159,19 +183,16 @@ TEST(TEST_TIMESTAMP, TIMESTAMP_WITH_NANO) {

for (size_t i = 0; i < values.size(); ++i) {
ASSERT_EQ(sizeof(TIMESTAMP_STRUCT), strlen_buffer[i]);
tm date{};

auto converted_time = values[i] / NANO_TO_SECONDS_DIVISOR;
GetTimeForSecondsSinceEpoch(date, converted_time);

ASSERT_EQ(buffer[i].year, 1900 + (date.tm_year));
ASSERT_EQ(buffer[i].month, date.tm_mon + 1);
ASSERT_EQ(buffer[i].day, date.tm_mday);
ASSERT_EQ(buffer[i].hour, date.tm_hour);
ASSERT_EQ(buffer[i].minute, date.tm_min);
ASSERT_EQ(buffer[i].second, date.tm_sec);
ASSERT_EQ(buffer[i].fraction, (values[i] % NANO_TO_SECONDS_DIVISOR));

ASSERT_EQ(buffer[i].year, 1900 + (expected[i].year));
ASSERT_EQ(buffer[i].month, expected[i].month + 1);
ASSERT_EQ(buffer[i].day, expected[i].day);
ASSERT_EQ(buffer[i].hour, expected[i].hour);
ASSERT_EQ(buffer[i].minute, expected[i].minute);
ASSERT_EQ(buffer[i].second, expected[i].second);
ASSERT_EQ(buffer[i].fraction, expected[i].fraction);
}
}

} // namespace flight_sql
} // namespace driver
10 changes: 3 additions & 7 deletions odbcabstraction/calendar_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "odbcabstraction/calendar_utils.h"

#include <boost/date_time/posix_time/posix_time.hpp>
#include <cstdint>
#include <ctime>

Expand All @@ -29,12 +30,7 @@ int64_t GetTodayTimeFromEpoch() {
}

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
}
date = boost::posix_time::to_tm(boost::posix_time::from_time_t(value));
}
} // namespace odbcabstraction
} // namespace driver

0 comments on commit e59ac76

Please sign in to comment.