[CALCITE-7639] Support bitwise right shift (>>) operator and RIGHTSHIFT function#5073
[CALCITE-7639] Support bitwise right shift (>>) operator and RIGHTSHIFT function#5073nielspardon wants to merge 3 commits into
Conversation
nielspardon
force-pushed
the
CALCITE-7639-bitwise-right-shift
branch
from
bb53cb6 to
a6d5390
Compare
nielspardon
changed the title
…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, 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.
Tests: SqlOperatorTest (operator + function forms), SqlFunctionsTest,
operator.iq, and the operator-precedence dump in SqlValidatorTest; docs in
site/_docs/reference.md.
nielspardon
force-pushed
the
CALCITE-7639-bitwise-right-shift
branch
from
a6d5390 to
d765854
Compare
… out shift operand checker
Address review feedback on the right shift PR:
* Parser.jj: replace the LOOKAHEAD(2) on the two '>' tokens with a
semantic lookahead that checks the tokens are immediately adjacent
(same line, adjacent columns). Previously '>>' matched even when the
two '>' were separated by whitespace, so 'a > > b' was parsed as a
right shift; it is now a syntax error, falling through to the single
'>' (greater-than) alternative.
* SqlStdOperatorTable: factor the duplicated operand type checker shared
by '<<', '>>', LEFTSHIFT and RIGHTSHIFT into a single
SHIFT_OPERAND_TYPE_CHECKER constant.
* SqlParserTest: add testShiftOperators covering '<<'/'>>' parsing,
precedence and left-associativity, and rejection of 'a > > b'.
| // 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); |
There was a problem hiding this comment.
Since LEFTSHIFT supports negative amounts, isn't RIGHTSHIFT(a, b) the same as LEFTSHIFT(a, -b)?
There was a problem hiding this 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.
…oken check
Address review feedback: factor the '>>' token-adjacency check used by the
right shift operator into a reusable SqlParserPos.adjacent(SqlParserPos)
method, rather than open-coding the line/column comparison in the parser.
* SqlParserPos: add adjacent(SqlParserPos), which reports whether two
positions are immediately adjacent (one ends exactly where the other
begins, with no characters in between).
* Parser.jj: add a plain pos(Token) helper and use
pos(getToken(1)).adjacent(pos(getToken(2))) in the BIT_RIGHT_SHIFT
lookahead. pos is a plain method (not JAVACODE) so it can be called
from generated lookahead code, which does not declare
throws ParseException.
* SqlParserPosTest: add a unit test for SqlParserPos.adjacent.
|
| * that is, one position ends exactly where the other begins, with no | ||
| * characters (such as whitespace) in between. | ||
| * | ||
| * <p>For example, the two {@code >} characters in {@code >>} are adjacent, |
There was a problem hiding this comment.
I would probably remove this part of the comment.
| | LOOKAHEAD({ getToken(1).kind == GT && getToken(2).kind == GT | ||
| && getToken(1).endLine == getToken(2).beginLine | ||
| && getToken(1).endColumn + 1 == getToken(2).beginColumn }) | ||
| && pos(getToken(1)).adjacent(pos(getToken(2))) }) |
There was a problem hiding this comment.
here you can use endsImmediatelyBefore, it's a bit more precise.
This would make "adjacent" dead code. I am not sure that function is as useful.
| * @return the shifted integer | ||
| */ | ||
| public static int rightShift(int x, int y) { | ||
| int shift = ((y % 32) + 32) % 32; // normalize to 0~31 |
There was a problem hiding this comment.
I would pull this into a helper positive_modulo function.
| * @param y the long shift amount | ||
| * @return the shifted value as long | ||
| */ | ||
| public static long rightShift(int x, long y) { |
There was a problem hiding this comment.
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.
| * Performs PostgresSQL-style bitwise shift on ULong. | ||
| * Overflow bits are masked to 64 bits (long shifts naturally truncate). | ||
| */ | ||
| public static ULong rightShift(ULong x, int y) { |
There was a problem hiding this comment.
why don't you need versions with long y for these?
3 participants
Jira Link
CALCITE-7639
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, mirroringLEFTSHIFT.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.