GH-45876: [C++][Acero] Make NormalizeTime order-preserving - #51299
Open
abelianbee wants to merge 1 commit into
Open
GH-45876: [C++][Acero] Make NormalizeTime order-preserving#51299abelianbee wants to merge 1 commit into
abelianbee wants to merge 1 commit into
Conversation
The bias was applied after widening to uint64_t, so a negative t sign-extended first and the addition wrapped back into the non-negative branch's range. That folded the signed domain 2-to-1 and inverted order at -1 -> 0. Flip the sign bit inside T's own width and zero-extend instead. The definition moves into the header so test translation units can instantiate it.
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
NormalizeTimeis documented as preserving order. For signed types it doesn't, because the bias is applied after the value has already been widened:A negative
tconverts touint64_tfirst and sign-extends to2^64 + t, so adding2^(W-1)wraps back into[0, 2^(W-1)), where the non-negative branch already maps. That folds the domain 2-to-1:NormalizeTime(t) == NormalizeTime(t + 2^(W-1))for every negativet. Over the full int16 domain only 32768 of 65536 outputs are distinct, and order inverts once, at-1 -> 0.NormalizeTime(INT64_MIN)andNormalizeTime(int64_t{0})are both 0.GetTimeroutes TIMESTAMP, DATE64 and TIME64 through int64 and DATE32/TIME32 through int32, and asof_join_node and sorted_merge_node both key rows on it, so any pre-epoch timestamp hits this.Two symptoms. The one in the issue is the out-of-order error, from input that is correctly sorted, such as
[-1000, 0, 1000]:The other is silent.
TolType::Acceptscompares differences of normalized values, and those are exact only when both operands have the same sign, so an asof join whose tolerance window straddles the epoch drops matches that are inside the window without raising anything. A left row at t=30 and a right row at t=-30 with a backward tolerance of 60 returns null.What changes are included in this PR?
Flip the sign bit inside
T's own width, then zero-extend. That is a strictly increasing bijection onto the same-width unsigned type, and zero-extension preserves order. UnsignedTstays the identity.The definition moves into the header. It was declared there and defined in the
.ccwith no explicit instantiation, so it only linked becauseGetTimeinstantiates it in that same TU, and no test TU could instantiate it at all. Explicit instantiations would work too if you would rather it stayed put.Are these changes tested?
New
time_series_util_test.ccon the existingutil_testtarget: exhaustive monotonicity and injectivity over int8, uint8, int16 and uint16, boundary sweeps for the wider types,INT64_MIN -> 0andINT64_MAX -> UINT64_MAX, and exactness of differences spanning zero. PlusTimesStraddlingEpochAreOrderedandToleranceWindowStraddlingEpochin asof_join_node_test.All six fail without the fix. With it, util_test 18 pass, asof_join_node_test 154 pass with the pre-existing BackpressureWithBatchesGen skip, sorted_merge_node_test 1 pass.
Are there any user-facing changes?
Asof joins and sorted merges over pre-epoch timestamps stop erroring and stop dropping rows. Normalized values are an internal key encoding, so no API change.
This PR contains a "Critical Fix". Asof joins over pre-epoch timestamps could silently return wrong results.