FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-45876: [C++][Acero] Make NormalizeTime order-preserving by abelianbee · Pull Request #51299 · apache/arrow · GitHub

/ arrow Public

GH-45876: [C++][Acero] Make NormalizeTime order-preserving - #51299

Closed
abelianbee wants to merge 1 commit into
apache:mainfrom
abelianbee:acero-normalize-time-ordering
Closed

GH-45876: [C++][Acero] Make NormalizeTime order-preserving#51299
abelianbee wants to merge 1 commit into
apache:mainfrom
abelianbee:acero-normalize-time-ordering

Conversation

abelianbee commented Sep 11, 2026
edited by github-actions Bot
Loading

Copy link
Copy Markdown

Rationale for this change

NormalizeTime is documented as preserving order. For signed types it doesn't, because the bias is applied after the value has already been widened:

uint64_t bias =
    std::is_signed<T>::value ? static_cast<uint64_t>(1) << (8 * sizeof(T) - 1) : 0;
return t < 0 ? static_cast<uint64_t>(t + bias) : static_cast<uint64_t>(t);

A negative t converts to uint64_t first and sign-extends to 2^64 + t, so adding 2^(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 negative t. Over the full int16 domain only 32768 of 65536 outputs are distinct, and order inverts once, at -1 -> 0. NormalizeTime(INT64_MIN) and NormalizeTime(int64_t{0}) are both 0.

GetTime routes 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]:

Invalid: AsofJoin does not allow out-of-order on-key values
  asof_join_node.cc:693  Advance()

The other is silent. TolType::Accepts compares 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. Unsigned T stays the identity.

The definition moves into the header. It was declared there and defined in the .cc with no explicit instantiation, so it only linked because GetTime instantiates 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.cc on the existing util_test target: exhaustive monotonicity and injectivity over int8, uint8, int16 and uint16, boundary sweeps for the wider types, INT64_MIN -> 0 and INT64_MAX -> UINT64_MAX, and exactness of differences spanning zero. Plus TimesStraddlingEpochAreOrdered and ToleranceWindowStraddlingEpoch in 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.

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.

Copy link
Copy Markdown

⚠️ GitHub issue #45876 has been automatically assigned in GitHub to PR creator.

abelianbee closed this by deleting the head repository Sep 11, 2026
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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant


Back | FazBrowse Home | New Git URL