Skip to content

Commit 1bec017

Browse files
nielspardonnielspardon
committed
[CALCITE-7639] Simplify shift helpers: endsImmediatelyBefore, Math.floorMod, unsigned-amount tests
Address further review feedback: * SqlParserPos: replace the symmetric adjacent(SqlParserPos) with the directional endsImmediatelyBefore(SqlParserPos), which is more precise for the right-shift token check and is now used directly by the parser. * SqlFunctions: replace the hand-rolled ((y % w) + w) % w shift-amount normalization with Math.floorMod(y, w) throughout leftShift/rightShift. * SqlOperatorTest: add tests showing a BIGINT shift amount is accepted while an unsigned shift amount is rejected at validation, for both the >> operator and the RIGHTSHIFT function.
1 parent 3fd6f5e commit 1bec017

5 files changed

Lines changed: 53 additions & 54 deletions

File tree

‎core/src/main/codegen/templates/Parser.jj‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8497,7 +8497,7 @@ SqlBinaryOperator BinaryRowOperator() :
84978497
// so "a > > b" is not treated as a right shift; otherwise we fall through to
84988498
// the single ">" (greater-than) alternative below.
84998499
| LOOKAHEAD({ getToken(1).kind == GT && getToken(2).kind == GT
8500-
&& pos(getToken(1)).adjacent(pos(getToken(2))) })
8500+
&& pos(getToken(1)).endsImmediatelyBefore(pos(getToken(2))) })
85018501
<GT> <GT> { return SqlStdOperatorTable.BIT_RIGHT_SHIFT; }
85028502
| <GT> { return SqlStdOperatorTable.GREATER_THAN; }
85038503
| <LT> { return SqlStdOperatorTable.LESS_THAN; }

‎core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3780,7 +3780,7 @@ private static ByteString binaryOperator(
37803780
* @return the shifted integer
37813781
*/
37823782
public static int leftShift(int x, int y) {
3783-
int shift = ((y % 32) + 32) % 32; // normalize to 0~31
3783+
int shift = Math.floorMod(y, 32); // normalize to 0~31
37843784
return y >= 0 ? x << shift : x >> shift; // arithmetic right shift
37853785
}
37863786

@@ -3794,7 +3794,7 @@ public static int leftShift(int x, int y) {
37943794
* @return the shifted long value
37953795
*/
37963796
public static long leftShift(long x, int y) {
3797-
int shift = ((y % 64) + 64) % 64; // normalize to 0~63
3797+
int shift = Math.floorMod(y, 64); // normalize to 0~63
37983798
return y >= 0 ? x << shift : x >> shift;
37993799
}
38003800

@@ -3806,7 +3806,7 @@ public static long leftShift(long x, int y) {
38063806
* @return the shifted value as long
38073807
*/
38083808
public static long leftShift(int x, long y) {
3809-
int shift = (int) (((y % 32) + 32) % 32); // normalize to 0~31
3809+
int shift = Math.floorMod(y, 32); // normalize to 0~31
38103810
return y >= 0 ? (long) x << shift : (long) x >> shift;
38113811
}
38123812

@@ -3828,7 +3828,7 @@ public static byte[] leftShift(byte[] bytes, int y) {
38283828

38293829
// PostgreSQL behavior: always treat as left shift with modulo arithmetic
38303830
// Negative y becomes equivalent positive shift
3831-
int shift = ((y % bitLen) + bitLen) % bitLen;
3831+
int shift = Math.floorMod(y, bitLen);
38323832

38333833
if (shift == 0) {
38343834
return bytes.clone();
@@ -3875,7 +3875,7 @@ public static ByteString leftShift(ByteString bytes, int y) {
38753875
* Overflow bits are masked to 8 bits.
38763876
*/
38773877
public static UByte leftShift(UByte x, int y) {
3878-
int shift = ((y % 8) + 8) % 8;
3878+
int shift = Math.floorMod(y, 8);
38793879
int val = x.byteValue() & 0xFF;
38803880
val = (y >= 0) ? (val << shift) & 0xFF : (val >> shift) & 0xFF;
38813881
return UByte.valueOf((byte) val);
@@ -3886,7 +3886,7 @@ public static UByte leftShift(UByte x, int y) {
38863886
* Overflow bits are masked to 16 bits.
38873887
*/
38883888
public static UShort leftShift(UShort x, int y) {
3889-
int shift = ((y % 16) + 16) % 16;
3889+
int shift = Math.floorMod(y, 16);
38903890
int val = x.shortValue() & 0xFFFF;
38913891
val = (y >= 0) ? (val << shift) & 0xFFFF : (val >> shift) & 0xFFFF;
38923892
return UShort.valueOf((short) val);
@@ -3897,7 +3897,7 @@ public static UShort leftShift(UShort x, int y) {
38973897
* Overflow bits are masked to 32 bits.
38983898
*/
38993899
public static UInteger leftShift(UInteger x, int y) {
3900-
int shift = ((y % 32) + 32) % 32;
3900+
int shift = Math.floorMod(y, 32);
39013901
long val = x.longValue() & 0xFFFFFFFFL;
39023902
val = (y >= 0) ? (val << shift) & 0xFFFFFFFFL : (val >> shift) & 0xFFFFFFFFL;
39033903
return UInteger.valueOf(val);
@@ -3908,7 +3908,7 @@ public static UInteger leftShift(UInteger x, int y) {
39083908
* Overflow bits are masked to 64 bits (long shifts naturally truncate).
39093909
*/
39103910
public static ULong leftShift(ULong x, int y) {
3911-
int shift = ((y % 64) + 64) % 64;
3911+
int shift = Math.floorMod(y, 64);
39123912
long val = x.longValue();
39133913
val = (y >= 0) ? val << shift : val >> shift;
39143914
return ULong.valueOf(val);
@@ -3922,7 +3922,7 @@ public static ULong leftShift(ULong x, int y) {
39223922
* @return the shifted integer
39233923
*/
39243924
public static int rightShift(int x, int y) {
3925-
int shift = ((y % 32) + 32) % 32; // normalize to 0~31
3925+
int shift = Math.floorMod(y, 32); // normalize to 0~31
39263926
return y >= 0 ? x >> shift : x << shift; // arithmetic right shift
39273927
}
39283928

@@ -3936,7 +3936,7 @@ public static int rightShift(int x, int y) {
39363936
* @return the shifted long value
39373937
*/
39383938
public static long rightShift(long x, int y) {
3939-
int shift = ((y % 64) + 64) % 64; // normalize to 0~63
3939+
int shift = Math.floorMod(y, 64); // normalize to 0~63
39403940
return y >= 0 ? x >> shift : x << shift;
39413941
}
39423942

@@ -3948,7 +3948,7 @@ public static long rightShift(long x, int y) {
39483948
* @return the shifted value as long
39493949
*/
39503950
public static long rightShift(int x, long y) {
3951-
int shift = (int) (((y % 32) + 32) % 32); // normalize to 0~31
3951+
int shift = Math.floorMod(y, 32); // normalize to 0~31
39523952
return y >= 0 ? (long) x >> shift : (long) x << shift;
39533953
}
39543954

@@ -3970,7 +3970,7 @@ public static byte[] rightShift(byte[] bytes, int y) {
39703970

39713971
// PostgreSQL behavior: always treat as right shift with modulo arithmetic
39723972
// Negative y becomes equivalent positive shift
3973-
int shift = ((y % bitLen) + bitLen) % bitLen;
3973+
int shift = Math.floorMod(y, bitLen);
39743974

39753975
if (shift == 0) {
39763976
return bytes.clone();
@@ -4017,7 +4017,7 @@ public static ByteString rightShift(ByteString bytes, int y) {
40174017
* Overflow bits are masked to 8 bits.
40184018
*/
40194019
public static UByte rightShift(UByte x, int y) {
4020-
int shift = ((y % 8) + 8) % 8;
4020+
int shift = Math.floorMod(y, 8);
40214021
int val = x.byteValue() & 0xFF;
40224022
val = (y >= 0) ? (val >> shift) & 0xFF : (val << shift) & 0xFF;
40234023
return UByte.valueOf((byte) val);
@@ -4028,7 +4028,7 @@ public static UByte rightShift(UByte x, int y) {
40284028
* Overflow bits are masked to 16 bits.
40294029
*/
40304030
public static UShort rightShift(UShort x, int y) {
4031-
int shift = ((y % 16) + 16) % 16;
4031+
int shift = Math.floorMod(y, 16);
40324032
int val = x.shortValue() & 0xFFFF;
40334033
val = (y >= 0) ? (val >> shift) & 0xFFFF : (val << shift) & 0xFFFF;
40344034
return UShort.valueOf((short) val);
@@ -4039,7 +4039,7 @@ public static UShort rightShift(UShort x, int y) {
40394039
* Overflow bits are masked to 32 bits.
40404040
*/
40414041
public static UInteger rightShift(UInteger x, int y) {
4042-
int shift = ((y % 32) + 32) % 32;
4042+
int shift = Math.floorMod(y, 32);
40434043
long val = x.longValue() & 0xFFFFFFFFL;
40444044
val = (y >= 0) ? (val >> shift) & 0xFFFFFFFFL : (val << shift) & 0xFFFFFFFFL;
40454045
return UInteger.valueOf(val);
@@ -4050,7 +4050,7 @@ public static UInteger rightShift(UInteger x, int y) {
40504050
* Overflow bits are masked to 64 bits (long shifts naturally truncate).
40514051
*/
40524052
public static ULong rightShift(ULong x, int y) {
4053-
int shift = ((y % 64) + 64) % 64;
4053+
int shift = Math.floorMod(y, 64);
40544054
long val = x.longValue();
40554055
val = (y >= 0) ? val >> shift : val << shift;
40564056
return ULong.valueOf(val);

‎core/src/main/java/org/apache/calcite/sql/parser/SqlParserPos.java‎

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -264,30 +264,16 @@ public boolean startsAt(SqlParserPos pos) {
264264
&& columnNumber == pos.columnNumber;
265265
}
266266

267-
/**
268-
* Returns whether this position is immediately adjacent to another position;
269-
* that is, one position ends exactly where the other begins, with no
270-
* characters (such as whitespace) in between.
271-
*
272-
* <p>For example, the two {@code >} characters in {@code >>} are adjacent,
273-
* whereas those in {@code > >} are not.
274-
*
275-
* @param pos position to compare against
276-
* @return whether this position and {@code pos} are immediately adjacent
277-
*/
278-
public boolean adjacent(SqlParserPos pos) {
279-
return endsImmediatelyBefore(pos) || pos.endsImmediatelyBefore(this);
280-
}
281-
282267
/**
283268
* Returns whether this position ends exactly one column before another
284-
* position begins, on the same line.
269+
* position begins, on the same line, with no characters (such as whitespace)
270+
* in between.
285271
*
286272
* @param pos position that may immediately follow this one
287273
* @return whether this position ends exactly one column before {@code pos}
288274
* begins, on the same line
289275
*/
290-
private boolean endsImmediatelyBefore(SqlParserPos pos) {
276+
public boolean endsImmediatelyBefore(SqlParserPos pos) {
291277
return endLineNumber == pos.lineNumber
292278
&& endColumnNumber + 1 == pos.columnNumber;
293279
}

‎core/src/test/java/org/apache/calcite/sql/parser/SqlParserPosTest.java‎

Lines changed: 18 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -25,35 +25,33 @@
2525
* Tests for {@link SqlParserPos}.
2626
*/
2727
public class SqlParserPosTest {
28-
/** Tests {@link SqlParserPos#adjacent(SqlParserPos)}. */
29-
@Test void testAdjacent() {
30-
// Two single-character positions in consecutive columns are adjacent,
31-
// like the two '>' of a '>>' token.
28+
/** Tests {@link SqlParserPos#endsImmediatelyBefore(SqlParserPos)}. */
29+
@Test void testEndsImmediatelyBefore() {
30+
// A single-character position ends immediately before the one in the next
31+
// column, like the two '>' of a '>>' token.
3232
final SqlParserPos col1 = new SqlParserPos(1, 1);
3333
final SqlParserPos col2 = new SqlParserPos(1, 2);
34-
assertThat(col1.adjacent(col2), is(true));
34+
assertThat(col1.endsImmediatelyBefore(col2), is(true));
3535

36-
// Adjacency is symmetric.
37-
assertThat(col2.adjacent(col1), is(true));
36+
// The relation is directional, not symmetric.
37+
assertThat(col2.endsImmediatelyBefore(col1), is(false));
3838

39-
// A gap between the positions (e.g. whitespace, like '> >') is not
40-
// adjacent.
39+
// A gap between the positions (e.g. whitespace, like '> >') does not
40+
// qualify.
4141
final SqlParserPos col3 = new SqlParserPos(1, 3);
42-
assertThat(col1.adjacent(col3), is(false));
43-
assertThat(col3.adjacent(col1), is(false));
42+
assertThat(col1.endsImmediatelyBefore(col3), is(false));
4443

45-
// A position is not adjacent to itself.
46-
assertThat(col1.adjacent(col1), is(false));
44+
// A position does not end immediately before itself.
45+
assertThat(col1.endsImmediatelyBefore(col1), is(false));
4746

48-
// A multi-column position is adjacent to the position that starts one
49-
// column after it ends.
47+
// A multi-column position ends immediately before the position that starts
48+
// one column after it ends.
5049
final SqlParserPos cols1To2 = new SqlParserPos(1, 1, 1, 2);
51-
assertThat(cols1To2.adjacent(col3), is(true));
52-
assertThat(cols1To2.adjacent(col2), is(false));
50+
assertThat(cols1To2.endsImmediatelyBefore(col3), is(true));
51+
assertThat(cols1To2.endsImmediatelyBefore(col2), is(false));
5352

54-
// Positions on different lines are never adjacent.
53+
// Positions on different lines never qualify.
5554
final SqlParserPos line2 = new SqlParserPos(2, 1);
56-
assertThat(new SqlParserPos(1, 5).adjacent(line2), is(false));
57-
assertThat(line2.adjacent(new SqlParserPos(1, 5)), is(false));
55+
assertThat(new SqlParserPos(1, 5).endsImmediatelyBefore(line2), is(false));
5856
}
5957
}

‎testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17206,6 +17206,15 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
1720617206
f.checkScalar("CAST(2147483648 AS INTEGER UNSIGNED) >> 31", "1", "INTEGER UNSIGNED NOT NULL");
1720717207
f.checkScalar("CAST(1 AS INTEGER UNSIGNED) >> -1", "2147483648", "INTEGER UNSIGNED NOT NULL");
1720817208

17209+
// A BIGINT shift amount is accepted (INTEGER family), but an unsigned shift
17210+
// amount is not: the second operand must be a signed integer type.
17211+
f.checkScalar("CAST(8 AS INTEGER) >> CAST(2 AS BIGINT)", "2", "INTEGER NOT NULL");
17212+
f.checkFails("^8 >> CAST(2 AS INTEGER UNSIGNED)^",
17213+
"Cannot apply '>>' to arguments of type '<INTEGER> >> <INTEGER UNSIGNED>'\\. "
17214+
+ "Supported form\\(s\\): '<INTEGER> >> <INTEGER>'\\n'<BINARY> >> <INTEGER>'\\n"
17215+
+ "'<UNSIGNED_NUMERIC> >> <INTEGER>'",
17216+
false);
17217+
1720917218
// === Negative shift counts (normalized via modulo, then shifted the other way) ===
1721017219
f.checkScalar("8 >> -1", "0", "INTEGER NOT NULL");
1721117220
f.checkScalar("16 >> -2", "0", "INTEGER NOT NULL");
@@ -17323,6 +17332,12 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
1732317332
f.checkFails("^RIGHTSHIFT(1.2, 2)^",
1732417333
"Cannot apply 'RIGHTSHIFT' to arguments of type 'RIGHTSHIFT\\(<DECIMAL\\(2, 1\\)>, <INTEGER>\\)'\\. Supported form\\(s\\): 'RIGHTSHIFT\\(<INTEGER>, <INTEGER>\\)'\\n'RIGHTSHIFT\\(<BINARY>, <INTEGER>\\)'\\n'RIGHTSHIFT\\(<UNSIGNED_NUMERIC>, <INTEGER>\\)'",
1732517334
false);
17335+
// A BIGINT shift amount is accepted, but an unsigned shift amount is not:
17336+
// the second argument must be a signed integer type.
17337+
f.checkScalar("RIGHTSHIFT(8, CAST(2 AS BIGINT))", "2", "INTEGER NOT NULL");
17338+
f.checkFails("^RIGHTSHIFT(8, CAST(2 AS INTEGER UNSIGNED))^",
17339+
"Cannot apply 'RIGHTSHIFT' to arguments of type 'RIGHTSHIFT\\(<INTEGER>, <INTEGER UNSIGNED>\\)'\\. Supported form\\(s\\): 'RIGHTSHIFT\\(<INTEGER>, <INTEGER>\\)'\\n'RIGHTSHIFT\\(<BINARY>, <INTEGER>\\)'\\n'RIGHTSHIFT\\(<UNSIGNED_NUMERIC>, <INTEGER>\\)'",
17340+
false);
1732617341

1732717342

1732817343
// === Nulls ===

0 commit comments

Comments
 (0)