You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/10/06 13:11:58 UTC
[GitHub] [iceberg] nastra opened a new pull request, #5928: API: Handle negative/zero during num-digits calculation
nastra opened a new pull request, #5928:
URL: https://github.com/apache/iceberg/pull/5928
this was discovered while reviewing https://github.com/apache/iceberg/pull/5908
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra closed pull request #5928: API: Handle negative/zero during num-digits calculation
Posted by GitBox <gi...@apache.org>.
nastra closed pull request #5928: API: Handle negative/zero during num-digits calculation
URL: https://github.com/apache/iceberg/pull/5928
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #5928: API: Handle negative/zero during num-digits calculation
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #5928:
URL: https://github.com/apache/iceberg/pull/5928#discussion_r989048785
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -291,7 +291,9 @@ private static String sanitize(Literal<?> literal) {
}
private static String sanitizeNumber(Number value, String type) {
- int numDigits = (int) Math.log10(value.doubleValue()) + 1;
+ // log10 of zero isn't defined and will result in negative infinity
+ int numDigits =
+ 0.0d == value.doubleValue() ? 1 : (int) Math.log10(Math.abs(value.doubleValue())) + 1;
Review Comment:
for `Double.MIN_VALUE` we'll get something like `(-322-digit-float)` because `Math.log10` produces a negative result. Not sure how much we care about `Double.MIN_VALUE`/`Float.MIN_VALUE` but we're not producing correct results for them
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #5928: API: Handle negative/zero during num-digits calculation
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5928:
URL: https://github.com/apache/iceberg/pull/5928
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5928: API: Handle negative/zero during num-digits calculation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5928:
URL: https://github.com/apache/iceberg/pull/5928#discussion_r989383675
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -291,7 +291,9 @@ private static String sanitize(Literal<?> literal) {
}
private static String sanitizeNumber(Number value, String type) {
- int numDigits = (int) Math.log10(value.doubleValue()) + 1;
+ // log10 of zero isn't defined and will result in negative infinity
+ int numDigits =
+ 0.0d == value.doubleValue() ? 1 : (int) Math.log10(Math.abs(value.doubleValue())) + 1;
Review Comment:
Good point. Right now, this is summarizing the numeric value, even if that's outside the range of actual digits we'd be able to store because of precision. I think it's probably fine for now though?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #5928: API: Handle negative/zero during num-digits calculation
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #5928:
URL: https://github.com/apache/iceberg/pull/5928#discussion_r988900777
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionUtil.java:
##########
@@ -63,6 +64,23 @@ public void testSanitizeIn() {
ExpressionUtil.toSanitizedString(Expressions.in("test", 34, 345)));
}
+ @Test
+ public void zeroAndNegativeNumberHandling() {
+ Assertions.assertThat(
+ ExpressionUtil.toSanitizedString(
+ Expressions.in(
+ "test",
+ 0,
+ Integer.MIN_VALUE,
+ Integer.MAX_VALUE,
+ Float.MIN_VALUE,
+ Float.MAX_VALUE,
+ Double.MIN_VALUE,
+ Double.MAX_VALUE)))
+ .isEqualTo(
+ "test IN ((1-digit-int), (10-digit-int), (10-digit-int), (45-digit-float), (39-digit-float), (324-digit-float), (309-digit-float))");
Review Comment:
I'm currently not sure if Float.MIN_VALUE / Double.MIN_VALUE are correctly calculated here
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org