nielspardon commented on code in PR #5073:
URL: https://github.com/apache/calcite/pull/5073#discussion_r3542870525
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -920,6 +922,17 @@ void populate1() {
// (e.g., x << y)
defineMethod(BIT_LEFT_SHIFT, BuiltInMethod.LEFT_SHIFT.method,
NullPolicy.STRICT);
+ // Right shift operations: shift bits to the right by specified amount.
+ // Supports integer, unsigned integer, and binary data types.
+ // Shift amount is normalized using modulo arithmetic based on data type
bit width.
+
+ // RIGHTSHIFT: Function call syntax for bitwise right shift operation
(e.g., RIGHTSHIFT(x, y))
+ defineMethod(RIGHTSHIFT, BuiltInMethod.RIGHT_SHIFT.method,
NullPolicy.STRICT);
Review Comment:
Good question, but no — they aren't equivalent under the current (released)
semantics. Counterexample, both values from the existing tests: `RIGHTSHIFT(8,
1)` = `4`, whereas `LEFTSHIFT(8, -1)` = `0`.
The reason is how the runtime handles the shift amount: it normalizes the
*magnitude* with `((amount % w) + w) % w` (where `w` is the type's bit width,
e.g. 32 for `INTEGER`) and uses only the amount's *sign* to pick the direction.
So for `LEFTSHIFT(a, -b)` the amount is `-b`, and its normalized magnitude is
`w - (b mod w)` — not `b`. In other words `LEFTSHIFT(a, -b)` shifts by `w - b`,
not by `b`. Concretely for `INTEGER`, `-1` becomes a shift of `31`:
`LEFTSHIFT(8, -1)` = `8 >> 31` = `0`, while `RIGHTSHIFT(8, 1)` = `8 >> 1` =
`4`. (BINARY differs again: the `byte[]` path ignores the sign entirely and
always shifts in its native direction.)
That negative-amount behavior of `LEFTSHIFT` shipped in 1.41.0/1.42.0
(CALCITE-7109), so making the equivalence hold would mean changing released
semantics — a separate change, out of scope here.
That said, your instinct points at real duplication:
`leftShift`/`rightShift` differ only in which way the ternary points. Happy to
factor the int/long/unsigned bodies through a shared private helper in a
follow-up (or here) to cut that down, without touching behavior.
--
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]
