[CALCITE-7639] Support bitwise right shift (>>) operator and RIGHTSHIFT function by nielspardon · Pull Request #5073 · apache/calcite

nielspardon · 2026-07-03T15:15:58Z

Jira Link

Changes Proposed

Mirrors the left-shift work in CALCITE-7109 to close a gap in the umbrella issue CALCITE-5087.

Adds:

>> (SqlStdOperatorTable.BIT_RIGHT_SHIFT), a signed/arithmetic right shift (Java >>), symmetric to << (precedence 32, left-assoc, ReturnTypes.ARG0_NULLABLE, InferTypes.FIRST_KNOWN, and the same INTEGER / BINARY / UNSIGNED_NUMERIC operand families).
RIGHTSHIFT(x, n) scalar function, mirroring LEFTSHIFT.
SqlFunctions.rightShift(...) runtime overloads (int, long, byte[], ByteString, and the joou unsigned types), plus BuiltInMethod.RIGHT_SHIFT and the RexImpTable registrations for both operator and function.

Unlike <<, a greedy >> token cannot be added: the lexer would also match the two > that close nested angle-bracket types (e.g. MAP<INT, MAP<INT, INT>>), breaking type parsing. >> is therefore recognized in expression context as two adjacent > tokens via LOOKAHEAD(2) in BinaryRowOperator, leaving type parsing unchanged. Because there is no dedicated token, the SQL advisor advertises > rather than >>, so SqlAdvisorTest is not modified.

Scope is limited to >> (arithmetic). The logical/fill-zero >>> (RIGHT_SHIFT_FILL_ZERO) remains a possible follow-up.

The code in this PR was generated with the help of AI.

Dwrite

LGTM. This is consistent with the LEFTSHIFT operator introduced in #4478 (parser-level resolution with precedence 32). Nice symmetric completion of the feature.

mihaibudiu · 2026-07-07T18:59:02Z

+      // 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);


Since LEFTSHIFT supports negative amounts, isn't RIGHTSHIFT(a, b) the same as LEFTSHIFT(a, -b)?

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.

mihaibudiu · 2026-07-08T17:59:44Z

+   * @param y the long shift amount
+   * @return the shifted value as long
+   */
+  public static long rightShift(int x, long y) {


you could only implement the long shift amount by casting any other type to long in the generated enumerable code.

Is there a test with a >> CAST(x AS UNSIGNED)? If not, there should be.

Done. All leftShift/rightShift overloads now take a long shift amount, so an int amount widens automatically during codegen (EnumUtils.call resolves by widening) and a single amount type covers every value type — which let me drop the redundant (int, long) overloads and answers "why no long-y for these?" too.

It also closes a real gap: there was no (long, long) overload, so an executed BIGINT >> BIGINT would have failed to resolve at code generation — only checkType (which doesn't execute) was covering it. I added executed BIGINT-by-BIGINT cases for both the operator and function forms.

On the unsigned shift amount: I added a test, and it's currently rejected at validation — the second operand's family is signed INTEGER, so a >> CAST(x AS INTEGER UNSIGNED) is a Cannot apply error today (documented via checkFails). I left that as-is, since allowing it would be a separate change that also touches the released LEFTSHIFT; happy to file a follow-up if you'd like unsigned amounts supported.

Please file an issue about unsigned amounts not being supported. I think you will agree that's not normal.
But if this implementation will need to be altered to support that better, maybe you can prepare it now.
I suspect the right way will be to cast any of these unsigned values to a long. The overflow for BIGINT UNSIGNED may require special handling.

Filed as CALCITE-7648: https://issues.apache.org/jira/browse/CALCITE-7648

I've kept this PR scoped to the current behaviour (an unsigned amount is rejected, which the existing checkFails tests pin down) and captured your suggested approach in the ticket: widen the operand type checker to accept UNSIGNED_NUMERIC for the second operand, cast the amount to long in the generated enumerable code, with special handling for a BIGINT UNSIGNED amount whose high bit is set (it exceeds Long.MAX_VALUE). Happy to pick that up as the follow-up.

nielspardon · 2026-07-09T08:01:30Z

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.

mihaibudiu · 2026-07-09T21:35:46Z

+    byte[] data2 = {(byte) 0x12, (byte) 0x34};
+    for (int shift = -18; shift <= 18; shift++) {
+      byte[] result = SqlFunctions.rightShift(data2.clone(), shift);
+      assertEquals(2, result.length);


you don't really look at the result, maybe you can compute the same using integers and check?

Done — the byte-array loops in testLeftShift/testRightShift now compute the expected result from the value's integer interpretation and assert the actual bytes, not just the length. (Doing this is what surfaced the little-endian byte order I flagged on the binary-test thread.)

mihaibudiu · 2026-07-09T21:37:44Z

+    f.checkScalar("RIGHTSHIFT(0, 100)", "0", "INTEGER NOT NULL");
+
+    // === Binary types ===
+    f.checkScalar("RIGHTSHIFT(CAST(X'FF' AS BINARY(1)), 1)", "7f", "BINARY(1) NOT NULL");


Is the cast necessary in these tests?
I would think x'FF' is a BINARY(1).
Is shift defined for VARBINARY?

you will need some tests with VARBINARY too

You're right — X'FF' is already BINARY(1) (and X'FFFF' is BINARY(2), etc.), so the cast was unnecessary. I dropped the CAST(… AS BINARY(n)) from both the >> and RIGHTSHIFT tests. Shift is defined for the whole BINARY family, so I also added VARBINARY coverage — including a pair that shows the shift width tracks the value's actual length (>> 8 is a no-op on a 1-byte value but a genuine shift on a 2-byte one).

One thing writing those tests made explicit, and I'd value your opinion on: the byte-array shift behaves as if the array were a little-endian integer (index 0 = least-significant byte). So RIGHTSHIFT(X'1234', 4) is 4103 and RIGHTSHIFT(X'FFFF', 8) is ff00, rather than the big-endian 0123 / 00ff one might expect for a binary string. This is the convention inherited from the existing LEFTSHIFT implementation and everything is self-consistent with it — but it is surprising. Do you think little-endian is the semantics we want here, or should binary shifts be big-endian? Happy to file a separate issue if it's worth revisiting.

I think this is somewhat related to https://issues.apache.org/jira/browse/CALCITE-7368, which however treats BINARY values as big-endian.

It may be nice to have CAST(int AS BINARY(N)) << x = CAST((int << x) AS BINARY(N)), but maybe this is not possible?

I think shifts should follow the semantics of an existing database which provides this operation on BINARY values. Since we have already left shifts, isn't the semantics imposed? We should have asked this question when we implemented left shift. We should have a design before we have an implementation.

mihaibudiu · 2026-07-09T21:40:23Z

 | * | BITOR(value1, value2)                          | Returns the bitwise OR of *value1* and *value2*. *value1* and *value2* must both be integer or binary values. Binary values must be of the same length.
 | * | BITXOR(value1, value2)                         | Returns the bitwise XOR of *value1* and *value2*. *value1* and *value2* must both be integer or binary values. Binary values must be of the same length.
-| * | LEFTSHIFT(value1, value2) | Returns the result of left-shifting *value1* by *value2* bits. *value1* can be integer, unsigned integer, or binary. For binary, the result has the same length as *value1*. The shift amount *value2* is normalized using modulo arithmetic based on the bit width of *value1*. For integers, this uses modulo 32; for binary types, it uses modulo (8 × byte_length). Negative shift amounts are converted to equivalent positive shifts through this modulo operation. For example, `LEFTSHIFT(1, -2)` returns `1073741824` (equivalent to `1 << 30`), and `LEFTSHIFT(8, -1)` returns `0` due to overflow.
+| * | LEFTSHIFT(value1, value2) | Returns the result of left-shifting *value1* by *value2* bits. *value1* can be integer, unsigned integer, or binary. For binary, the result has the same length as *value1*. The shift amount *value2* is normalized using modulo arithmetic based on the bit width of *value1*. For integers, this uses modulo 32; for binary types, it uses modulo (8 × byte_length). The sign of *value2* selects the direction: a non-negative amount shifts left, and a negative amount shifts right by the normalized magnitude. For example, `LEFTSHIFT(1, -2)` returns `0` (a right shift by 30), and `LEFTSHIFT(8, -1)` returns `0`.


this definition is not entirely clear to me for a type like VARBINARY or VARBINARY(10). Is it the dynamic length of the binary string?

Clarified. It's 8 × N where N is the actual length in bytes of the value — for a variable-length VARBINARY that's the length of the value itself, not its declared maximum. While there I also fixed the direction wording, which only held for integer/unsigned types: for binary the shift is always in the named direction and a negative amount just folds into [0, 8 × N) by the same modulo (e.g. RIGHTSHIFT(X'F0', -4) = 0f).

nielspardon · 2026-07-13T10:33:27Z

Agreed — design first. I dug into the current behaviour and the precedents:

Our byte[] shift treats the array as little-endian and modulo-wraps the amount. The "PostgreSQL behavior" comment is misattributed: PostgreSQL doesn't define <</>> on bytea — only on bit strings, which are big-endian, length-preserving, zero-filled and don't wrap. MySQL's shifts operate on the integer value, not the bytes.
CALCITE-7368 made CAST(int AS BINARY) big-endian, so today CAST and shift disagree. Your identity CAST(int AS BINARY(N)) << x = CAST((int << x) AS BINARY(N)) holds only under big-endian + zero-fill + no-wrap, and even then only for 0 ≤ x < 8·N and matching width (Java's native int << masks the amount mod 32, so it diverges at x ≥ 32).
Binary left shift shipped in 1.41.0/1.42.0, so flipping its endianness is a released-behaviour change that deserves its own discussion.

So rather than cement the little-endian semantics in a new operator, I've descoped BINARY/VARBINARY from >> and RIGHTSHIFT in this PR — it now covers only the integer/unsigned surface, and a binary operand is rejected at validation (pinned by checkFails). I filed CALCITE-7651 to settle binary-shift endianness for both << and >> together, where I'll propose big-endian to match CALCITE-7368 and PostgreSQL bit strings. Binary left shift is left exactly as released.

Does that split work for you?

mihaibudiu · 2026-07-13T16:00:11Z

Yes, doing BINARY separately looks fine.

mihaibudiu · 2026-07-23T17:20:38Z

what is the status of this PR?

nielspardon · 2026-07-23T17:31:43Z

what is the status of this PR?

I dropped the Binary portion and since then it is waiting for hopefully a final round of reviews. Is there anything open from your side?

mihaibudiu · 2026-07-23T18:10:06Z

You should ask for a new review when warranted, there's too much stuff going on to assume that someone scans this queue
I will try to review it soon

nielspardon · 2026-07-23T18:16:57Z

You should ask for a new review when warranted, there's too much stuff going on to assume that someone scans this queue I will try to review it soon

apologies for the misunderstanding. I was also busy with other things. I appreciate you checking in.

mihaibudiu

This looks fine to me. I think you can squash so we can merge.

…FT function Mirrors the left-shift work in CALCITE-7109 to close a gap in the umbrella issue CALCITE-5087. Adds: * `>>` (SqlStdOperatorTable.BIT_RIGHT_SHIFT), a signed/arithmetic right shift (Java `>>`), symmetric to `<<` (precedence 32, left-assoc, ReturnTypes.ARG0_NULLABLE, InferTypes.FIRST_KNOWN). * `RIGHTSHIFT(x, n)` scalar function, mirroring `LEFTSHIFT`. * SqlFunctions.rightShift(...) runtime overloads (int, long, and the joou unsigned types), plus BuiltInMethod.RIGHT_SHIFT and the RexImpTable registrations for both operator and function. Operands are limited to integer and unsigned numeric types. Unlike `<<`, binary (BINARY/VARBINARY) right shift is intentionally rejected at validation until the endianness of bitwise shifts on binary is settled; that follow-up is tracked in CALCITE-7651. A greedy `>>` token cannot be added: the lexer would also match the two `>` that close nested angle-bracket types (e.g. MAP<INT, MAP<INT, INT>>), breaking type parsing. `>>` is therefore recognized in expression context as two adjacent `>` tokens via LOOKAHEAD(2) in BinaryRowOperator, leaving type parsing unchanged. Because there is no dedicated token, the SQL advisor advertises `>` rather than `>>`, so SqlAdvisorTest is not modified. Scope is limited to `>>` (arithmetic). The logical/fill-zero `>>>` (RIGHT_SHIFT_FILL_ZERO) remains a possible follow-up. Tests: SqlOperatorTest (operator + function forms), SqlFunctionsTest, operator.iq, and the operator-precedence dump in SqlValidatorTest; docs in site/_docs/reference.md.

sonarqubecloud · 2026-07-24T06:35:13Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
97.5% Coverage on New Code
25.9% Duplication on New Code

See analysis details on SonarQube Cloud

nielspardon · 2026-07-24T06:59:55Z

This looks fine to me. I think you can squash so we can merge.

I squashed it.

nielspardon force-pushed the CALCITE-7639-bitwise-right-shift branch from bb53cb6 to a6d5390 Compare July 3, 2026 15:28

nielspardon changed the title ~~[CALCITE-7639] support bitwise right shift (>>) operator and RIGHTSHIFT function~~ [CALCITE-7639] Support bitwise right shift (>>) operator and RIGHTSHIFT function Jul 3, 2026

nielspardon force-pushed the CALCITE-7639-bitwise-right-shift branch from a6d5390 to d765854 Compare July 3, 2026 15:44

Dwrite reviewed Jul 5, 2026

View reviewed changes

nielspardon mentioned this pull request Jul 6, 2026

feat(isthmus): map RIGHTSHIFT to Substrait shift_right substrait-io/substrait-java#986

Merged

mihaibudiu reviewed Jul 6, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java Outdated

Comment thread core/src/main/codegen/templates/Parser.jj Outdated

nielspardon requested a review from mihaibudiu July 7, 2026 14:41

mihaibudiu reviewed Jul 7, 2026

View reviewed changes

nielspardon requested a review from mihaibudiu July 8, 2026 09:42

mihaibudiu reviewed Jul 8, 2026

View reviewed changes

nielspardon requested a review from mihaibudiu July 9, 2026 06:38

nielspardon force-pushed the CALCITE-7639-bitwise-right-shift branch from bab2663 to 0d85f00 Compare July 9, 2026 07:06

mihaibudiu reviewed Jul 9, 2026

View reviewed changes

nielspardon force-pushed the CALCITE-7639-bitwise-right-shift branch from 63ec5dd to dc399d6 Compare July 13, 2026 12:27

mihaibudiu approved these changes Jul 23, 2026

View reviewed changes

mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 23, 2026

nielspardon force-pushed the CALCITE-7639-bitwise-right-shift branch from dc399d6 to f6075db Compare July 24, 2026 05:59

mihaibudiu merged commit cb24d30 into apache:main Jul 24, 2026
20 checks passed

Uh oh!

Conversation

nielspardon commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jira Link

Changes Proposed

Uh oh!

Dwrite left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nielspardon commented Jul 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielspardon commented Jul 13, 2026

Uh oh!

mihaibudiu commented Jul 13, 2026

Uh oh!

mihaibudiu commented Jul 23, 2026

Uh oh!

nielspardon commented Jul 23, 2026

Uh oh!

mihaibudiu commented Jul 23, 2026

Uh oh!

nielspardon commented Jul 23, 2026

Uh oh!

mihaibudiu left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Jul 24, 2026

Quality Gate passed

Uh oh!

nielspardon commented Jul 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nielspardon commented Jul 3, 2026 •

edited

Loading

Dwrite left a comment •

edited

Loading