[SPARK-26508][Core][SQL] Address warning messages in Java reported at lgtm.com #23420

kiszk · 2018-12-31T10:35:31Z

What changes were proposed in this pull request?

This PR addresses warning messages in Java files reported at lgtm.com.

lgtm.com provides automated code review of Java/Python/JavaScript files for OSS projects. Here are warning messages regarding Apache Spark project.

This PR addresses the following warnings:

Result of multiplication cast to wider type
Implicit narrowing conversion in compound assignment
Boxed variable is never null
Useless null check

NOTE: Potential input resource leak looks false positive for now.

How was this patch tested?

Existing UTs

viirya · 2018-12-31T14:44:44Z

core/src/main/scala/org/apache/spark/package.scala

Is this safe change? Looks like behavior change when throwing exception.

Thank you, this is my mistake. I will revert this.

srowen · 2018-12-31T14:40:01Z

common/network-common/src/main/java/org/apache/spark/network/util/ByteUnit.java

We need a space after casts. I suppose the point is to avoid overflow when the result is going to be a double anyway, at the cost of precision maybe. I tested, and the precision issue won't matter, but then again, there's no reason this method should return double. There is no such thing as 0.5 bytes.

I think we can just fix that for Spark 3; its value is used as a long or int everywhere anyway.

While here, I think I'd also fix the expressions like (long) Math.pow(1024L, 2L) above. The multipliers should be 1L << 10, 1L << 20, ... 1L << 50. There's again no point there in using floating point math.

I agree with chaniging the precision. I will use long here.
I will also use 1L << 10, ... in enum ByteUnit.

Hm, really? what does it return? type-wise the numbers should easily fit into a long. 2 TiB = 2199023255552 bytes. I'm missing where the failure is here.

srowen · 2018-12-31T14:41:03Z

common/network-common/src/main/java/org/apache/spark/network/util/TransportConf.java

Write 100.0 here for extra clarity, and put spaces after all the casts here. Otherwise, what's the issue with this expression?

lgtm.com and I are afraind (...) * chunkFetchHandlerThreadsPercent may cause overflow since it is int * int. After this change, it will be double * double.

Oh it's because it's really (threads * percent) / 100. Maybe this can be simplified by breaking out the (this.serverThreads() ...) into a local variable, then writing as threads * (percent / 100.0)

oops looks like I missed this comment, sorry for the duplicate.

srowen · 2018-12-31T14:41:53Z

core/src/main/scala/org/apache/spark/package.scala

What's the purpose of this change?

ditto, I will revert this.

srowen · 2018-12-31T14:46:13Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/xml/UDFXPathUtil.java

I'd put the cast on line 183 and make n an int. length and next are nonnegative ints.

srowen · 2018-12-31T14:47:52Z

launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java

You could probably just improve this by creating this class name string once above line 321, and putting something like "no message" instead of null, while we're here.

SparkQA · 2018-12-31T15:07:51Z

Test build #100588 has finished for PR 23420 at commit 34f0008.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-12-31T16:18:47Z

reopen this

kiszk · 2018-12-31T16:19:09Z

retest this please

SparkQA · 2018-12-31T20:15:05Z

Test build #100598 has finished for PR 23420 at commit cf5c927.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

holdenk

I'm not sure about the cast with the division, could we simplify it?

holdenk · 2019-01-01T00:19:39Z

common/network-common/src/main/java/org/apache/spark/network/util/TransportConf.java

    return (int)Math.ceil(
     (this.serverThreads() > 0 ? this.serverThreads() : 2 * NettyRuntime.availableProcessors()) *
-     chunkFetchHandlerThreadsPercent/(double)100);
+     (double) chunkFetchHandlerThreadsPercent/100);


I'm not so sure about moving this cast, why are doing that? Would it make more sense to write this as chunkFetchHandlerThreadsPercent / 100.0 ?

fix test failure

SparkQA · 2019-01-01T08:14:17Z

Test build #100606 has finished for PR 23420 at commit 557a940.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2019-01-01T14:30:36Z

retest this please

srowen

Looks fine pending tests, with one tiny request

srowen · 2019-01-01T15:21:09Z

...alyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/UnsafeArrayWriter.java


  private long getElementOffset(int ordinal) {
-    return startingOffset + headerInBytes + ordinal * elementSize;
+    return startingOffset + headerInBytes + ordinal * (long)elementSize;


Nit: space after cast

SparkQA · 2019-01-01T19:07:27Z

Test build #100623 has finished for PR 23420 at commit 557a940.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-01-02T04:13:19Z

Test build #100628 has finished for PR 23420 at commit 3df0a0a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2019-01-02T04:37:47Z

Merged to master

… lgtm.com ## What changes were proposed in this pull request? This PR addresses warning messages in Java files reported at [lgtm.com](https://lgtm.com). [lgtm.com](https://lgtm.com) provides automated code review of Java/Python/JavaScript files for OSS projects. [Here](https://lgtm.com/projects/g/apache/spark/alerts/?mode=list&severity=warning) are warning messages regarding Apache Spark project. This PR addresses the following warnings: - Result of multiplication cast to wider type - Implicit narrowing conversion in compound assignment - Boxed variable is never null - Useless null check NOTE: `Potential input resource leak` looks false positive for now. ## How was this patch tested? Existing UTs Closes apache#23420 from kiszk/SPARK-26508. Authored-by: Kazuaki Ishizaki <[email protected]> Signed-off-by: Sean Owen <[email protected]>

viirya reviewed Dec 31, 2018

View reviewed changes

srowen requested changes Dec 31, 2018

View reviewed changes

kiszk closed this Dec 31, 2018

kiszk force-pushed the SPARK-26508 branch from 34f0008 to 240817b Compare December 31, 2018 15:44

kiszk deleted the SPARK-26508 branch December 31, 2018 16:03

kiszk restored the SPARK-26508 branch December 31, 2018 16:05

initial commit

cf5c927

kiszk reopened this Dec 31, 2018

holdenk reviewed Jan 1, 2019

View reviewed changes

address review comments

557a940

fix test failure

srowen requested changes Jan 1, 2019

View reviewed changes

address review comment

3df0a0a

srowen approved these changes Jan 2, 2019

View reviewed changes

srowen closed this in 79b0548 Jan 2, 2019

[SPARK-26508][Core][SQL] Address warning messages in Java reported at lgtm.com #23420

[SPARK-26508][Core][SQL] Address warning messages in Java reported at lgtm.com #23420

Uh oh!

Conversation

kiszk commented Dec 31, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Dec 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

kiszk commented Dec 31, 2018

Uh oh!

kiszk commented Dec 31, 2018

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

holdenk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 1, 2019

Uh oh!

kiszk commented Jan 1, 2019

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 1, 2019

Uh oh!

SparkQA commented Jan 2, 2019

Uh oh!

srowen commented Jan 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kiszk Dec 31, 2018 •

edited

Loading