yashmayya commented on code in PR #18658:
URL: https://github.com/apache/pinot/pull/18658#discussion_r3352834621
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RelToPlanNodeConverter.java:
##########
@@ -950,8 +950,17 @@ public static ColumnDataType
convertToColumnDataType(RelDataType relDataType) {
case TINYINT:
case SMALLINT:
case INTEGER:
+ // Calcite 1.41+ (CALCITE-1466) parses unsigned integer types under
BABEL conformance. Pinot has no unsigned
+ // storage, so map each to the narrowest signed type that holds its full
range: UTINYINT (0..2^8-1) and
+ // USMALLINT (0..2^16-1) fit in INT; UINTEGER (0..2^32-1) needs LONG (a
signed INT would wrap values above 2^31).
+ case UTINYINT:
+ case USMALLINT:
return isArray ? ColumnDataType.INT_ARRAY : ColumnDataType.INT;
case BIGINT:
+ case UINTEGER:
+ // UBIGINT (0..2^64-1) has no wider signed type, so values above
Long.MAX_VALUE wrap (two's-complement) - this is
+ // unavoidable and acceptable since Pinot has no unsigned storage type.
+ case UBIGINT:
Review Comment:
Good catch — agreed, and fixed. `BIGINT UNSIGNED` (`UBIGINT`) now fails fast
at planning instead of being mapped to a lossy `LONG`:
`convertToColumnDataType` throws a clear *"Unsigned BIGINT is not supported …
CAST to BIGINT or DECIMAL instead"* error.
I kept the other unsigned types accepted, since those map **losslessly** and
don't have the wrap problem — `TINYINT`/`SMALLINT UNSIGNED` → `INT`, `INTEGER
UNSIGNED` → `LONG`. `UBIGINT` is the only one with no signed Pinot type wide
enough for its full `0..2⁶⁴−1` range, so it's the one that has to be rejected.
The rejection is applied consistently across every site that touched the
type — both `(P)RelToPlanNodeConverter.convertToColumnDataType`, the
single-stage `DataTypeConversionFunctions.cast`, `TypeSystem.deriveSumType`,
and `ArithmeticFunctionUtils.normalizeNumericType` — and is covered by
regression tests (`QueryCompilationTest#testUnsignedBigintCastIsRejected` for
both the column- and literal-cast planning paths, plus updated
`RelToPlanNodeConverterTest`/`PRelToPlanNodeConverterTest`/`DataTypeConversionFunctionsTest`
unit assertions). PR description updated too. Thanks!
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
