Jackie-Jiang commented on code in PR #18940:
URL: https://github.com/apache/pinot/pull/18940#discussion_r3547187082
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/PinotDataType.java:
##########
@@ -834,12 +834,22 @@ public Long toInternal(Object value) {
STRING {
@Override
public int toInt(Object value) {
- return Integer.parseInt(value.toString().trim());
+ String trimmedStr = value.toString().trim();
+ try {
+ return Integer.parseInt(trimmedStr);
+ } catch (NumberFormatException e) {
+ return new BigDecimal(trimmedStr).intValue();
+ }
Review Comment:
Valid concern — the `BigDecimal` fallback would silently wrap on overflow
instead of throwing (e.g. `"2147483648"` / `"9223372036854775808"`). Rather
than add range checks, I dropped the `PinotDataType` change entirely:
`STRING`/`JSON` `toInt`/`toLong` stay strict and keep throwing, including on
out-of-range values — a non-integer string cast to an integral type should
throw, consistent with SQL semantics. The `dateTimeConvert` regression is now
fixed at its source: string epoch inputs route through the pre-existing
`DateTimeFormatSpec.fromFormatToMillis(String)` `BigDecimal` path, so this file
is no longer part of the PR.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/PinotDataType.java:
##########
@@ -834,12 +834,22 @@ public Long toInternal(Object value) {
STRING {
@Override
public int toInt(Object value) {
- return Integer.parseInt(value.toString().trim());
+ String trimmedStr = value.toString().trim();
+ try {
+ return Integer.parseInt(trimmedStr);
+ } catch (NumberFormatException e) {
+ return new BigDecimal(trimmedStr).intValue();
+ }
}
@Override
public long toLong(Object value) {
- return Long.parseLong(value.toString().trim());
+ String trimmedStr = value.toString().trim();
+ try {
+ return Long.parseLong(trimmedStr);
+ } catch (NumberFormatException e) {
+ return new BigDecimal(trimmedStr).longValue();
+ }
Review Comment:
Valid concern — the `BigDecimal` fallback would silently wrap on overflow
instead of throwing (e.g. `"2147483648"` / `"9223372036854775808"`). Rather
than add range checks, I dropped the `PinotDataType` change entirely:
`STRING`/`JSON` `toInt`/`toLong` stay strict and keep throwing, including on
out-of-range values — a non-integer string cast to an integral type should
throw, consistent with SQL semantics. The `dateTimeConvert` regression is now
fixed at its source: string epoch inputs route through the pre-existing
`DateTimeFormatSpec.fromFormatToMillis(String)` `BigDecimal` path, so this file
is no longer part of the PR.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/PinotDataType.java:
##########
@@ -894,12 +904,22 @@ public String convert(Object value, PinotDataType
sourceType) {
JSON {
@Override
public int toInt(Object value) {
- return Integer.parseInt(value.toString().trim());
+ String trimmedStr = value.toString().trim();
+ try {
+ return Integer.parseInt(trimmedStr);
+ } catch (NumberFormatException e) {
+ return new BigDecimal(trimmedStr).intValue();
+ }
Review Comment:
Valid concern — the `BigDecimal` fallback would silently wrap on overflow
instead of throwing (e.g. `"2147483648"` / `"9223372036854775808"`). Rather
than add range checks, I dropped the `PinotDataType` change entirely:
`STRING`/`JSON` `toInt`/`toLong` stay strict and keep throwing, including on
out-of-range values — a non-integer string cast to an integral type should
throw, consistent with SQL semantics. The `dateTimeConvert` regression is now
fixed at its source: string epoch inputs route through the pre-existing
`DateTimeFormatSpec.fromFormatToMillis(String)` `BigDecimal` path, so this file
is no longer part of the PR.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/PinotDataType.java:
##########
@@ -894,12 +904,22 @@ public String convert(Object value, PinotDataType
sourceType) {
JSON {
@Override
public int toInt(Object value) {
- return Integer.parseInt(value.toString().trim());
+ String trimmedStr = value.toString().trim();
+ try {
+ return Integer.parseInt(trimmedStr);
+ } catch (NumberFormatException e) {
+ return new BigDecimal(trimmedStr).intValue();
+ }
}
@Override
public long toLong(Object value) {
- return Long.parseLong(value.toString().trim());
+ String trimmedStr = value.toString().trim();
+ try {
+ return Long.parseLong(trimmedStr);
+ } catch (NumberFormatException e) {
+ return new BigDecimal(trimmedStr).longValue();
+ }
Review Comment:
Valid concern — the `BigDecimal` fallback would silently wrap on overflow
instead of throwing (e.g. `"2147483648"` / `"9223372036854775808"`). Rather
than add range checks, I dropped the `PinotDataType` change entirely:
`STRING`/`JSON` `toInt`/`toLong` stay strict and keep throwing, including on
out-of-range values — a non-integer string cast to an integral type should
throw, consistent with SQL semantics. The `dateTimeConvert` regression is now
fixed at its source: string epoch inputs route through the pre-existing
`DateTimeFormatSpec.fromFormatToMillis(String)` `BigDecimal` path, so this file
is no longer part of the PR.
--
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]
