Fix for DX-86729 & DX-90059 #3
Conversation
indigophox
force-pushed
the
pre-epoch-date-and-timestamp-fixes
branch
2 times, most recently
from
e59ac76
to
1e870c7
Compare
| // 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)); |
There was a problem hiding this comment.
For this expression we already know that std::abs() will negate the value to get a positive since we've already checked that units_since_epoch < 0. Would it be better to just negate? Negation is usually its own instruction.
There was a problem hiding this comment.
Abs should be its own instruction too; I think abs is clearer as to the intention?
I /should/ probably comment (so people don't have to check the Standard) that it's going to round towards zero.
There was a problem hiding this comment.
What I should do though is fix this equivalent to #3 (review) (below comment about explanation) so it doesn't have the undesirable case where it adds too much (and ends up taking modulo of the (positive) divisor by itself...).
There was a problem hiding this comment.
What I should do though is fix this equivalent to #3 (review) (below comment about explanation) so it doesn't have the undesirable case where it adds too much (and ends up taking modulo of the (positive) divisor by itself...).
Did this, and commented wrt rounding direction.
| const auto converted_result_seconds = value / divisor; | ||
| const auto converted_result_seconds = | ||
| (value < 0) | ||
| ? ((value - (divisor - 1)) / divisor) |
There was a problem hiding this comment.
This formula needs an explanation.
There was a problem hiding this comment.
There is a bunch of code duplication that's not very apparent between this method and CalculateFraction().
Can we make GetConversionToSecondsDivisor() take and return constexprs and make divisor a constexpr? (This should help to both optimize out the switch and treat the divisor as a constant)?
Can we make the shifting calculation a function (that takes in constexprs) with a signature like:
int64_t ShiftForNegativeIfNeeded(constexpr TimeUnit unit, int64_t value)
which conditionally checks if value is negative and runs the shift calculation if needed (and calls into GetConversionToSecondsDivisor(), which should be inlined).
Then have this method call ShiftForNegativeIfNeeded() and CalculateFraction call GetConversionToScondsDivisor() and ShiftForNegativeIfNeeded().
This would reduce code duplication, allow the compiler to optimize out more constexprs, and make it more apparent that the same calculation is being used repeatedly.
There was a problem hiding this comment.
I am inclined to factor some of that as the more obvious floor_div() and the portable modulo which is how it would be written if C++ was useful :) Also any reason for constexpr functions instead of just inline?
There was a problem hiding this comment.
inline is just a suggestion. constexpr has a stronger guarantee (which is that constants-only will be used), making the function more friendly for constant folding.
indigophox
force-pushed
the
pre-epoch-date-and-timestamp-fixes
branch
6 times, most recently
from
fd46ca8
to
b83e984
Compare
| if (units_since_epoch < 0) | ||
| units_since_epoch += driver::odbcabstraction::MICRO_TO_SECONDS_DIVISOR * | ||
| std::abs((units_since_epoch - (driver::odbcabstraction::MICRO_TO_SECONDS_DIVISOR - 1)) | ||
| / driver::odbcabstraction::MICRO_TO_SECONDS_DIVISOR); | ||
| return (units_since_epoch % | ||
| driver::odbcabstraction::MICRO_TO_SECONDS_DIVISOR) * 1000; | ||
| case TimeUnit::NANO: | ||
| // 1000 nanoseconds = 1 microsecond. |
There was a problem hiding this comment.
This comment is wrong (or at least irrelevant). Should say that fraction is already in nanoseconds.
| @@ -32,23 +34,36 @@ 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) { | |||
There was a problem hiding this comment.
Is it possible to eliminate this switch entirely? If we make the shifting calculation its own function (described in my other comment) then we are pretty much left with just the code to scale the fraction to nanoseconds.
Can that be expressed like units_since_epoch % (NANOS_TO_SECONDS_DIVISOR / GetSecondsDivisor()). The right-hand of the mod operator should get optimized out into a constant and the switch would be eliminated. (might have gotten the division order backwards).
There was a problem hiding this comment.
More or less yes, and likewise with the return statements where I wish (easy enough to fix) the 10**n magic numbers were derived as constexpr. However we're left with the shift-to-force-rounding-direction, which is in the opposite direction in different places here (floor div vs ceiling div) so I would either be duplicating the function to handle that detail both ways OR negating the operands to get the same effect which is pretty hacky (and buggy for unchecked signed ints).
indigophox
force-pushed
the
pre-epoch-date-and-timestamp-fixes
branch
2 times, most recently
from
3f1b8e9
to
8ed5bdf
Compare
DX-86729 (Windows-only) fix applied to all platforms for behaviour consistency +Add DATE and TIMESTAMP accessor tests +Handle inconsistent implementation-defined negative mod +Fixed negative epoch time division; fixed tests accordingly +Fixed (pending CI tests) handling of values near INT64_MIN +Added test for case near INT64_MIN nanoseconds. Change-Id: I16e4fe9befbff2327608bca92216a670492c8e44
indigophox
force-pushed
the
pre-epoch-date-and-timestamp-fixes
branch
from
8ed5bdf
to
a4ae3be
Compare
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
Change-Id: I16e4fe9befbff2327608bca92216a670492c8e44