nielspardon commented on PR #5073: URL: https://github.com/apache/calcite/pull/5073#issuecomment-4922925418
Pushed a couple of fixes found while doing a local review of this PR: **1. `RIGHTSHIFT` corrupted `BIGINT UNSIGNED` values with the high bit set.** `SqlFunctions.rightShift(ULong, long)` used an arithmetic `>>` on the raw (signed) long, so it sign-extended instead of shifting logically — e.g. right-shifting `2^63` by 4 returned `17870283321406128128` instead of `576460752303423488`. Unlike the `UByte`/`UShort`/`UInteger` paths (which mask to a non-negative domain before shifting), `ULong` holds the full 64 bits, so it needs a logical `>>>`. Fixed that, plus the same defect in `leftShift(ULong, long)`'s negative-amount branch. **2. Negative-shift docs were inaccurate.** The reference docs said negative amounts are "converted to equivalent positive shifts," and the `LEFTSHIFT(1, -2)` example was wrong. A negative amount actually shifts in the *opposite* direction by the normalized magnitude, so `RIGHTSHIFT(1, -2)` is `1073741824` (a left shift by 30) and `LEFTSHIFT(1, -2)` is `0` (a right shift by 30). Corrected both entries. I also added coverage for the high-bit `BIGINT UNSIGNED` cases and for the negative-amount direction (the previous negative tests only hit values that overflow to 0, so they didn't distinguish the direction). These run through the executing operator-test path, so they exercise the actual runtime rather than only validating types — which is how the `ULong` bug surfaced. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
