nielspardon commented on code in PR #5073:
URL: https://github.com/apache/calcite/pull/5073#discussion_r3556733429
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -17158,6 +17168,204 @@ private static void
checkLogicalOrFunc(SqlOperatorFixture f) {
f.checkNull("LEFTSHIFT(CAST(NULL AS INTEGER UNSIGNED), 2)");
}
+ /**
+ * Test cases for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7639";>[CALCITE-7639]
+ * Support bitwise right shift (>>) operator and RIGHTSHIFT
function</a>.
+ */
+ @Test void testRightShiftScalarFunc() {
+ final SqlOperatorFixture f = fixture();
+ f.setFor(SqlStdOperatorTable.BIT_RIGHT_SHIFT, VmName.EXPAND);
+
+ // === Basic functionality ===
+ f.checkScalar("8 >> 2", "2", "INTEGER NOT NULL");
+ f.checkScalar("1024 >> 10", "1", "INTEGER NOT NULL");
+ f.checkScalar("0 >> 5", "0", "INTEGER NOT NULL");
+
+ // === Type coercion and signed (arithmetic) behavior ===
+ f.checkScalar("CAST(16 AS INTEGER) >> CAST(3 AS BIGINT)", "2", "INTEGER
NOT NULL");
+ f.checkScalar("-20 >> 2", "-5", "INTEGER NOT NULL");
+ f.checkScalar("-40 >> 3", "-5", "INTEGER NOT NULL");
+ f.checkScalar("CAST(-20 AS TINYINT) >> CAST(2 AS TINYINT)", "-5", "TINYINT
NOT NULL");
+
+ // === Verify return type matches first argument type ===
+ f.checkType("CAST(8 AS TINYINT) >> CAST(2 AS TINYINT)", "TINYINT NOT
NULL");
+ f.checkType("CAST(8 AS SMALLINT) >> CAST(2 AS SMALLINT)", "SMALLINT NOT
NULL");
+ f.checkType("CAST(8 AS INTEGER) >> CAST(2 AS INTEGER)", "INTEGER NOT
NULL");
+ f.checkType("CAST(8 AS BIGINT) >> CAST(2 AS BIGINT)", "BIGINT NOT NULL");
+ f.checkScalar("CAST(8 AS BIGINT) >> CAST(2 AS BIGINT)", "2", "BIGINT NOT
NULL");
+
+ // === BigInt shifts with explicit BIGINT inputs
(arithmetic/sign-preserving) ===
+ f.checkScalar("CAST(4611686018427387904 AS BIGINT) >> 62", "1", "BIGINT
NOT NULL"); // 2^62
+ f.checkScalar("CAST(9223372036854775807 AS BIGINT) >> 1",
+ BigInteger.valueOf(Long.MAX_VALUE).shiftRight(1).toString(), "BIGINT
NOT NULL");
+ f.checkScalar("CAST(-1 AS BIGINT) >> 63", "-1", "BIGINT NOT NULL"); //
sign bit preserved
+ f.checkScalar("CAST(-1 AS BIGINT) >> 1", "-1", "BIGINT NOT NULL");
+ f.checkScalar("CAST(1000000000 AS BIGINT) >> 5", "31250000", "BIGINT NOT
NULL");
+
+ // === Shift amount normalized using modulo of the bit width ===
+ f.checkScalar("CAST(1024 AS BIGINT) >> 64", "1024", "BIGINT NOT NULL"); //
64 % 64 = 0
+ f.checkScalar("CAST(1024 AS BIGINT) >> 74", "1", "BIGINT NOT NULL"); // 74
% 64 = 10
+ f.checkScalar("1 >> 32", "1", "INTEGER NOT NULL"); // 32 % 32 = 0
+ f.checkScalar("123 >> 60", "0", "INTEGER NOT NULL"); // 60 % 32 = 28
+
+ // === Unsigned types ===
+ f.checkScalar("CAST(252 AS TINYINT UNSIGNED) >> 2", "63", "TINYINT
UNSIGNED NOT NULL");
+ f.checkScalar("CAST(65280 AS SMALLINT UNSIGNED) >> 8", "255", "SMALLINT
UNSIGNED NOT NULL");
+ f.checkScalar("CAST(4294901760 AS INTEGER UNSIGNED) >> 16", "65535",
+ "INTEGER UNSIGNED NOT NULL");
+ f.checkScalar("CAST(2147483648 AS INTEGER UNSIGNED) >> 31", "1", "INTEGER
UNSIGNED NOT NULL");
+ f.checkScalar("CAST(1 AS INTEGER UNSIGNED) >> -1", "2147483648", "INTEGER
UNSIGNED NOT NULL");
+ // BIGINT UNSIGNED with the high bit set (2^63, built via 1 << 63): the
+ // right shift must be logical, not arithmetic (the raw long is negative).
+ f.checkScalar("CAST(1 AS BIGINT UNSIGNED) << 63 >> 4",
+ "576460752303423488", "BIGINT UNSIGNED NOT NULL");
+
+ // A BIGINT shift amount is accepted (INTEGER family), but an unsigned
shift
+ // amount is not: the second operand must be a signed integer type.
+ f.checkScalar("CAST(8 AS INTEGER) >> CAST(2 AS BIGINT)", "2", "INTEGER NOT
NULL");
+ f.checkFails("^8 >> CAST(2 AS INTEGER UNSIGNED)^",
+ "Cannot apply '>>' to arguments of type '<INTEGER> >> <INTEGER
UNSIGNED>'\\. "
+ + "Supported form\\(s\\): '<INTEGER> >> <INTEGER>'\\n'<BINARY> >>
<INTEGER>'\\n"
+ + "'<UNSIGNED_NUMERIC> >> <INTEGER>'",
+ false);
+
+ // === Negative shift counts (normalized via modulo, then shifted the
other way) ===
+ // A negative right shift shifts left by the normalized magnitude (here
+ // 32 - 2 = 30); it is not the same as a left shift by the given amount.
+ f.checkScalar("1 >> -2", "1073741824", "INTEGER NOT NULL"); // 1 << 30
+ f.checkScalar("8 >> -1", "0", "INTEGER NOT NULL");
+ f.checkScalar("16 >> -2", "0", "INTEGER NOT NULL");
+
+ // === Shift by zero and large shifts ===
+ f.checkScalar("0 >> 32", "0", "INTEGER NOT NULL");
+ f.checkScalar("0 >> 100", "0", "INTEGER NOT NULL");
+
+ // === Binary type tests ===
+ f.checkScalar("CAST(X'FF' AS BINARY(1)) >> 1", "7f", "BINARY(1) NOT NULL");
+ f.checkScalar("CAST(X'F0' AS BINARY(1)) >> 4", "0f", "BINARY(1) NOT NULL");
+ f.checkScalar("CAST(X'08' AS BINARY(1)) >> 3", "01", "BINARY(1) NOT NULL");
+ f.checkScalar("CAST(X'00' AS BINARY(1)) >> 5", "00", "BINARY(1) NOT NULL");
+
+ f.checkScalar("CAST(X'FFFF' AS BINARY(2)) >> 1", "ff7f", "BINARY(2) NOT
NULL");
+ f.checkScalar("CAST(X'1234' AS BINARY(2)) >> 4", "4103", "BINARY(2) NOT
NULL");
+ f.checkScalar("CAST(X'1234' AS BINARY(2)) >> 8", "3400", "BINARY(2) NOT
NULL");
+
+ f.checkScalar("CAST(X'FF' AS BINARY(1)) >> 8", "ff", "BINARY(1) NOT NULL");
+ f.checkScalar("CAST(X'FFFF' AS BINARY(2)) >> 16", "ffff", "BINARY(2) NOT
NULL");
+
+ f.checkScalar("CAST(X'ABCD' AS BINARY(2)) >> 0", "abcd", "BINARY(2) NOT
NULL");
+ f.checkScalar("CAST(X'123456' AS BINARY(3)) >> 4", "416305", "BINARY(3)
NOT NULL");
+ f.checkScalar("CAST(X'0001' AS BINARY(2)) >> 1", "8000", "BINARY(2) NOT
NULL");
+ f.checkScalar("CAST(X'8000' AS BINARY(2)) >> 1", "4000", "BINARY(2) NOT
NULL");
+ f.checkScalar("CAST(X'F0' AS BINARY(1)) >> -4", "0f", "BINARY(1) NOT
NULL");
+
+ // === Invalid argument types ===
+ f.checkFails("^1.2 >> 2^",
+ "Cannot apply '>>' to arguments of type '<DECIMAL\\(2, 1\\)> >>
<INTEGER>'\\. Supported "
+ + "form\\(s\\): '<INTEGER> >> <INTEGER>'\\n'<BINARY> >>
<INTEGER>'\\n'<UNSIGNED_NUMERIC> "
+ + ">> <INTEGER>'",
+ false);
+
+ // === Null propagation ===
+ f.checkNull("CAST(NULL AS INTEGER) >> 5");
+ f.checkNull("10 >> CAST(NULL AS INTEGER)");
+ f.checkNull("CAST(NULL AS INTEGER) >> CAST(NULL AS INTEGER)");
+ f.checkNull("CAST(NULL AS INTEGER UNSIGNED) >> 2");
+ }
+
+ @Test void testRightShiftFunctionCall() {
+ final SqlOperatorFixture f = fixture();
+ f.setFor(SqlStdOperatorTable.BIT_RIGHT_SHIFT, VmName.EXPAND);
+
+ // === Basic functionality ===
+ f.checkScalar("RIGHTSHIFT(8, 2)", "2", "INTEGER NOT NULL");
+ f.checkScalar("RIGHTSHIFT(1024, 10)", "1", "INTEGER NOT NULL");
+ f.checkScalar("RIGHTSHIFT(0, 5)", "0", "INTEGER NOT NULL");
+
+ // === Type coercion and signed (arithmetic) behavior ===
+ f.checkScalar("RIGHTSHIFT(CAST(16 AS INTEGER), CAST(3 AS BIGINT))", "2",
"INTEGER NOT NULL");
+ f.checkScalar("RIGHTSHIFT(-20, 2)", "-5", "INTEGER NOT NULL");
+ f.checkScalar("RIGHTSHIFT(-40, 3)", "-5", "INTEGER NOT NULL");
+ f.checkScalar("RIGHTSHIFT(CAST(-20 AS TINYINT), CAST(2 AS TINYINT))",
"-5", "TINYINT NOT NULL");
+
+ // === Verify return type matches first argument type ===
+ f.checkType("RIGHTSHIFT(CAST(8 AS TINYINT), CAST(2 AS TINYINT))", "TINYINT
NOT NULL");
+ f.checkType("RIGHTSHIFT(CAST(8 AS SMALLINT), CAST(2 AS SMALLINT))",
"SMALLINT NOT NULL");
+ f.checkType("RIGHTSHIFT(CAST(8 AS INTEGER), CAST(2 AS INTEGER))", "INTEGER
NOT NULL");
+ f.checkType("RIGHTSHIFT(CAST(8 AS BIGINT), CAST(2 AS BIGINT))", "BIGINT
NOT NULL");
+ f.checkScalar("RIGHTSHIFT(CAST(8 AS BIGINT), CAST(2 AS BIGINT))", "2",
"BIGINT NOT NULL");
+
+ // === BigInt shifts with explicit BIGINT inputs ===
+ f.checkScalar("RIGHTSHIFT(CAST(4611686018427387904 AS BIGINT), 62)", "1",
"BIGINT NOT NULL");
+ f.checkScalar("RIGHTSHIFT(CAST(9223372036854775807 AS BIGINT), 1)",
+ BigInteger.valueOf(Long.MAX_VALUE).shiftRight(1).toString(), "BIGINT
NOT NULL");
+ f.checkScalar("RIGHTSHIFT(CAST(-1 AS BIGINT), 63)", "-1", "BIGINT NOT
NULL");
+ f.checkScalar("RIGHTSHIFT(CAST(-1 AS BIGINT), 1)", "-1", "BIGINT NOT
NULL");
+ f.checkScalar("RIGHTSHIFT(CAST(1000000000 AS BIGINT), 5)", "31250000",
"BIGINT NOT NULL");
+
+ // === Shift amount normalized using modulo of the bit width ===
+ f.checkScalar("RIGHTSHIFT(CAST(1024 AS BIGINT), 64)", "1024", "BIGINT NOT
NULL");
+ f.checkScalar("RIGHTSHIFT(CAST(1024 AS BIGINT), 74)", "1", "BIGINT NOT
NULL");
+ f.checkScalar("RIGHTSHIFT(1, 32)", "1", "INTEGER NOT NULL");
+ f.checkScalar("RIGHTSHIFT(123, 60)", "0", "INTEGER NOT NULL");
+
+ // === Unsigned types ===
+ f.checkScalar("RIGHTSHIFT(CAST(252 AS TINYINT UNSIGNED), 2)", "63",
+ "TINYINT UNSIGNED NOT NULL");
+ f.checkScalar("RIGHTSHIFT(CAST(65280 AS SMALLINT UNSIGNED), 8)", "255",
+ "SMALLINT UNSIGNED NOT NULL");
+ f.checkScalar("RIGHTSHIFT(CAST(4294901760 AS INTEGER UNSIGNED), 16)",
"65535",
+ "INTEGER UNSIGNED NOT NULL");
+ f.checkScalar("RIGHTSHIFT(CAST(2147483648 AS INTEGER UNSIGNED), 31)", "1",
+ "INTEGER UNSIGNED NOT NULL");
+ f.checkScalar("RIGHTSHIFT(CAST(1 AS INTEGER UNSIGNED), -1)", "2147483648",
+ "INTEGER UNSIGNED NOT NULL");
+
+ // === Negative shifts ===
+ f.checkScalar("RIGHTSHIFT(8, -1)", "0", "INTEGER NOT NULL");
+ f.checkScalar("RIGHTSHIFT(16, -2)", "0", "INTEGER NOT NULL");
+
+ // === Large shifts ===
+ f.checkScalar("RIGHTSHIFT(0, 32)", "0", "INTEGER NOT NULL");
+ 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");
Review Comment:
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.
##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -2068,6 +2068,34 @@ private long sqlTimestamp(String str) {
SqlFunctions.leftShift(new byte[]{(byte) 0x40, (byte) 0x00}, 1));
}
+ @Test void testRightShift() {
+ // Test 1-byte array
+ byte[] data1 = {(byte) 0xF0}; // 11110000
+ for (int shift = -10; shift <= 10; shift++) {
+ byte[] result = SqlFunctions.rightShift(data1.clone(), shift);
+ // Just verify it doesn't crash and returns correct length
+ assertEquals(1, result.length);
+ }
+
+ // Test 2-byte array
+ byte[] data2 = {(byte) 0x12, (byte) 0x34};
+ for (int shift = -18; shift <= 18; shift++) {
+ byte[] result = SqlFunctions.rightShift(data2.clone(), shift);
+ assertEquals(2, result.length);
Review Comment:
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.)
##########
site/_docs/reference.md:
##########
@@ -2973,7 +2973,8 @@ In the following:
| * | BITAND(value1, value2) | Returns the bitwise AND
of *value1* and *value2*. *value1* and *value2* must both be integer or binary
values. Binary values must be of the same length.
| * | 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`.
Review Comment:
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`).
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3914,6 +3914,148 @@ public static ULong leftShift(ULong x, int y) {
return ULong.valueOf(val);
}
+ /**
+ * Performs PostgresSQL-style bitwise shift on a 32-bit integer.
+ *
+ * @param x the integer value to shift
+ * @param y the shift amount (positive: right shift, negative: left shift)
+ * @return the shifted integer
+ */
+ public static int rightShift(int x, int y) {
+ int shift = ((y % 32) + 32) % 32; // normalize to 0~31
+ return y >= 0 ? x >> shift : x << shift; // arithmetic right shift
+ }
+
+
+ // ----------------- long -----------------
+ /**
+ * Performs PostgresSQL-style bitwise shift on a 64-bit long value.
+ *
+ * @param x the long value to shift
+ * @param y the shift amount
+ * @return the shifted long value
+ */
+ public static long rightShift(long x, int y) {
+ int shift = ((y % 64) + 64) % 64; // normalize to 0~63
+ return y >= 0 ? x >> shift : x << shift;
+ }
+
+ /**
+ * Performs PostgresSQL-style bitwise shift on an int value with a long
shift amount.
+ *
+ * @param x the int value to shift
+ * @param y the long shift amount
+ * @return the shifted value as long
+ */
+ public static long rightShift(int x, long y) {
Review Comment:
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.
--
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]
