[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user cloud-fan commented on the issue: https://github.com/apache/spark/pull/21481 thanks, merging to master/2.3! --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/21481 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/91922/ Test PASSed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/21481 Merged build finished. Test PASSed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/21481 **[Test build #91922 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91922/testReport)** for PR 21481 at commit [`a87c417`](https://github.com/apache/spark/commit/a87c4171324cc7f413ac18993d398c41fb345d43). * This patch passes all tests. * This patch merges cleanly. * This patch adds no public classes. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/21481 @cloud-fan addressed all of the possible integer overflows detected by SpotBugs. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user cloud-fan commented on the issue: https://github.com/apache/spark/pull/21481 @kiszk when you were running findBugs locally, did you find more overflow bugs that are not present in this PR? Let's put all discovered overflow bugs in this PR and have another PR to integrate findBugs with maven. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/21481 Thank you for your comment. I will create another PR for integrating findBugs/SpotBugs into maven. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user JoshRosen commented on the issue: https://github.com/apache/spark/pull/21481 Let's merge this as-is and do the build improvements in a separate PR. That's important because we may want to backport the overflow fix to maintenance branches and may want to do so independent of the build changes. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/21481 Since I found an plug-in for maven, I will also include a patch to add findBugs/SpotBugs into maven in this PR. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/21481 Since it is Java bytecode analysis, it is available for Scala code, too. In my quick test, findBugs overlooked a possible overflow. On the other hand, findBugs found another redundant null check. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user cloud-fan commented on the issue: https://github.com/apache/spark/pull/21481 is findBugs available for scala code as well? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/21481 @JoshRosen @cloud-fan Here is an update. I have just apply `findBugs` to `OffHeapColumnVector.java` and `UnsafeArrayData.java`. In `OffHeapColumnVector.java`, most of possible overflows are detected. But, not all. In `UnsafeArrayData.java`, two possible overflows are detected. Line 86 and 456. I overlooked Line 86. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/21481 Good questions. For 2, at first I found one of these issues when I looked at a file. Then, I ran `grep` command with `long .*=.*\*` and `long .*=.*\+` in `.java` file. Then, I picked them up manually. It looks labor-intensive. For 3, here is my thought. [`SpotBugs`](https://spotbugs.github.io/) may be a good candidate to check it. SpotBug is a successor of [`findBugs`](https://findbugs.sourceforge.net/). When I ran `FindBugs` before, I found some problems regarding possible overflow and then made a PR that was integrated. On the other hand, these issues may not be detected at that time. I will look at SpotBugs after my presentation at SAIS will be finished :) --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user cloud-fan commented on the issue: https://github.com/apache/spark/pull/21481 +1 on @JoshRosen 's ideas. I've already seen several overflow fixes from @kiszk , it will be good if we have some tools to check it, even we need to run the tool manually. One idea may be to force to use java 8's safe math functions: https://docs.oracle.com/javase/8/docs/api/java/lang/Math.html#addExact-int-int- --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user JoshRosen commented on the issue: https://github.com/apache/spark/pull/21481 Hey @kiszk, thanks for tracking this down. This change looks good to me. I have a couple of questions, mostly aimed towards figuring out how we can categorically solve this problem: 1. What's the impact of this issue? Have you observed actual crashes or silent data corruption caused by this problem? I ask because it looks like this could be a plausible cause of a data corruption bug that I've been investigating. 2. Are these five files the only occurrence of this problem or are there potentially others? I've noticed that all of the files modified here are `.java` files: is that a coincidence or is that the result of running some Java linting tool? If the latter, is it possible that we have similar issues in other files which we haven't found yet? 3. Do you have any ideas for how we can categorically prevent this class of problem in the future? Are there compiler plugins or linters which can warn on these cases and turn them into compile-time errors? Or code-review checklists that we can employ to more easily spot these potential overflows? This is a hard problem, but I think it's worth brainstorming on categorical solutions since this isn't the first time we've hit this class of problem (but hopefully it can be the last!). I think we should definitely backport this fix, at least to 2.3 and possibly earlier. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/21481 cc @cloud-fan --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/21481 cc @ueshin @hvanhovell --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/21481 Merged build finished. Test PASSed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/21481 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/91404/ Test PASSed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/21481 **[Test build #91404 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91404/testReport)** for PR 21481 at commit [`324fd5c`](https://github.com/apache/spark/commit/324fd5ccb73c8017f5537031db21b687ac1ca27a). * This patch passes all tests. * This patch merges cleanly. * This patch adds no public classes. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/21481 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/3768/ Test PASSed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/21481 Merged build finished. Test PASSed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21481: [SPARK-24452][SQL][Core] Avoid possible overflow in int ...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/21481 **[Test build #91404 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91404/testReport)** for PR 21481 at commit [`324fd5c`](https://github.com/apache/spark/commit/324fd5ccb73c8017f5537031db21b687ac1ca27a). --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
