Skip to content

Conversation

@kiszk
Copy link
Member

@kiszk kiszk commented Dec 27, 2015

Compilation error caused due to string concatenations that are not a constant
Use raw string literal to avoid string concatenations

https://amplab.cs.berkeley.edu/jenkins/view/Spark-Packaging/job/Spark-Master-Maven-Snapshots/1293/

use raw string literal to avoid string concatenations
@SparkQA
Copy link

SparkQA commented Dec 27, 2015

Test build #48348 has finished for PR 10488 at commit 9658c3b.

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

@hvanhovell
Copy link
Contributor

There a quite a few builds passing which are using string concatenation in combination with ExpressionDescription:
#10489
#10418
#10402

Maybe something else is causing this?

@kiszk
Copy link
Member Author

kiszk commented Dec 27, 2015

Thanks for letting me know them. I will check them.

@kiszk
Copy link
Member Author

kiszk commented Dec 27, 2015

@hvanhovell, a set of following two conditions causes this compilation failure:

  1. Use more than one string concatenations
  2. Use scala 2.11 compiler (open question is that this failure occurs by using scala 2.10 or 2.11 compilers on Jenkins

I can reproduce this compilation error by using the following command:

$ java -version
openjdk version "1.8.0_65"
OpenJDK Runtime Environment (build 1.8.0_65-b17)
OpenJDK 64-Bit Server VM (build 25.65-b01, mixed mode)
$ build/mvn -Dscala-2.11 -Pyarn -Phadoop-2.4 -DskipTests clean package install -pl sql/catalyst

vectorijk added a commit to vectorijk/spark that referenced this pull request Dec 28, 2015
switch some usage in ExpressionDescription to raw string literal
@hvanhovell
Copy link
Contributor

More people are having this problem. Lets use raw strings then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit style. I guess this was needed to stay within 100 charaters?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this keeps within 100 characters at a line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style below make newline display more reasonable.

s"""first line
  | second line
""".stripMargin

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that work under 2.11?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just tried this; it doesn't work unfortunately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yhuai , the above line did not cause the same issue. This is because its length is less than 100 and it has only one concatenation.

I checked by inserting // scalastyle:off before the annotation and // scalastyle:on after the annotation as follows:

// scalastyle:off
@ExpressionDescription(
  usage = "_FUNC_(input, bitLength) - Returns a checksum of SHA-2 family as a hex string of the " +
    "input. SHA-224, SHA-256, SHA-384, and SHA-512 are supported. Bit length of 0 is equivalent to 256",
  extended = "> SELECT _FUNC_('Spark', 0);\n " +
    "'529bc3b07127ecb7e53a4dcf1991d9152c24537d919178022b2c42657f79a26b'")
// scalastyle:on

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kiszk I see. How about we remove concatenation from both usage and extended? We can just use multi-line string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use the multi-line string, we need to be careful when adding \n. This does not work. I already hit this issue when I do the fix in another related PR: #10418

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yhuai, it sounds good to use multi-line string for usage and extended. How about this policy?

  1. First, we use raw string literal to remove concatenations.
  2. If we still want to use a line with more than 100 characters, // scalastyle:off and // scalastyle:on can be used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In another way, I am trying to reproduce this compilation error in a standalone test case. However, I cannot reproduce this failure (i.e. compilations are successfully finished).

$ cat ExpressionDescription.java 
import java.lang.annotation.*;

@Retention(RetentionPolicy.RUNTIME)
public @interface ExpressionDescription {
    String usage() default "_FUNC_ is undocumented";
    String extended() default "No example for _FUNC_.";
}
$ cat test.scala
@ExpressionDescription(
  usage = "_FUNC_(input, bitLength) - Returns a checksum of SHA-2 family as a hex string of the " +
    "input. SHA-224, SHA-256, SHA-384, and SHA-512 are supported. Bit length of 0 is equivalent " +
    "to 256",
  extended = "> SELECT _FUNC_('Spark', 0);\n " +
    "'529bc3b07127ecb7e53a4dcf1991d9152c24537d919178022b2c42657f79a26b'")
class test {
  def func() : Unit = {
    val str = "abc" + "123" + "XYZ"
  }
}
$ javac ExpressionDescription.java 
$ ~/scala-2.11.7/bin/scalac test.scala 
$

@hvanhovell
Copy link
Contributor

@yhuai can you take a look at this?

vectorijk added a commit to vectorijk/spark that referenced this pull request Dec 29, 2015
switch some usage in ExpressionDescription to raw string literal
@yhuai
Copy link
Contributor

yhuai commented Dec 29, 2015

@kiszk Thank you for the investigation. Yeah, let's use multi-line string literals. If we have to have a line with more than 100 characters, let's use // scalastyle:off line.size.limit and // scalastyle:on line.size.limit to just bypass the line number requirement.

@yhuai
Copy link
Contributor

yhuai commented Dec 29, 2015

Can you update the pr? Once it passes jenkins, I will merge it. Thanks!

@kiszk
Copy link
Member Author

kiszk commented Dec 29, 2015

@yhuai, do you mean that I would update all of the string concatenation in @ExpressionDescription by using multi-line string literals rather than only the original one?
If so, I will do this update.

@yhuai
Copy link
Contributor

yhuai commented Dec 29, 2015

Let me merge this one first to fix the spark master maven snapshot. Then, can you create another jira to update other places?

@asfgit asfgit closed this in 8e629b1 Dec 29, 2015
@kiszk
Copy link
Member Author

kiszk commented Dec 29, 2015

I see. I will create another JIRA entry to update other usages.

asfgit pushed a commit that referenced this pull request Jan 7, 2016
…ed in @ExpressionDescription

Use multi-line string literals for ExpressionDescription with ``// scalastyle:off line.size.limit`` and ``// scalastyle:on line.size.limit``

The policy is here, as describe at #10488

Let's use multi-line string literals. If we have to have a line with more than 100 characters, let's use ``// scalastyle:off line.size.limit`` and ``// scalastyle:on line.size.limit`` to just bypass the line number requirement.

Author: Kazuaki Ishizaki <[email protected]>

Closes #10524 from kiszk/SPARK-12580.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants