Skip to content

Conversation

@jaceklaskowski
Copy link
Contributor

What changes were proposed in this pull request?

Fix for the error introduced in c59abad:

/Users/jacek/dev/oss/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:626: error: annotation argument needs to be a constant; found: "_FUNC_(str) - ".+("Returns str, with the first letter of each word in uppercase, all other letters in ").+("lowercase. Words are delimited by white space.")
    "Returns str, with the first letter of each word in uppercase, all other letters in " +
                                                                                          ^

How was this patch tested?

Local build

jaceklaskowski referenced this pull request Apr 6, 2016
…lowercasing rest of string

## What changes were proposed in this pull request?

Current, SparkSQL `initCap` is using `toTitleCase` function. However, `UTF8String.toTitleCase` implementation changes only the first letter and just copy the other letters: e.g. sParK --> SParK. This is the correct implementation `toTitleCase`.
```
hive> select initcap('sParK');
Spark
```
```
scala> sql("select initcap('sParK')").head
res0: org.apache.spark.sql.Row = [SParK]
```

This PR updates the implementation of `initcap` using `toLowerCase` and `toTitleCase`.

## How was this patch tested?

Pass the Jenkins tests (including new testcase).

Author: Dongjoon Hyun <[email protected]>

Closes #12175 from dongjoon-hyun/SPARK-14402.
@marmbrus
Copy link
Contributor

marmbrus commented Apr 6, 2016

ok to test

@SparkQA
Copy link

SparkQA commented Apr 6, 2016

Test build #55069 has finished for PR 12192 at commit 218bf51.

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

@jaceklaskowski
Copy link
Contributor Author

Is this really the patch to cause the issue or is this more an interim hiccup?

@marmbrus
Copy link
Contributor

marmbrus commented Apr 6, 2016

@tdas another streaming failure

@dongjoon-hyun
Copy link
Member

Hi, @jaceklaskowski and @marmbrus .
Thank you for fixing this. I didn't notice there is such a problem.
Sorry for late response!
I have another PR having the same syntax. I will fix that, too.
Thank you again.

@srowen
Copy link
Member

srowen commented Apr 7, 2016

Jenkins retest this please

@SparkQA
Copy link

SparkQA commented Apr 7, 2016

Test build #55207 has finished for PR 12192 at commit 218bf51.

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

@dongjoon-hyun
Copy link
Member

Hi, All.
The original code is made by me, so I feel guilty. :)

I think Jenkins build does not show additional information for this PR since our Jenkins build on master branches works fine.

And, we know the JDK7 does not have this issue according to the Spark mailing list. So, I ran the following command and found the current master branch compiles without problem. This seems not a Scala compilation problem.

spark:master$ git clean -fdx
spark:master$ java -version
java version "1.8.0_77"
Java(TM) SE Runtime Environment (build 1.8.0_77-b03)
Java HotSpot(TM) 64-Bit Server VM (build 25.77-b03, mixed mode)
spark:master$ ./build/mvn -Pyarn -Phadoop-2.6 -Dhadoop.version=2.7.2 -Phive -Phive-thriftserver -DskipTests clean compile
...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 03:38 min
[INFO] Finished at: 2016-04-07T10:29:20-07:00
[INFO] Final Memory: 76M/871M
[INFO] ------------------------------------------------------------------------

After that, just package also works fine.

spark:master$ ./build/mvn -Pyarn -Phadoop-2.6 -Dhadoop.version=2.7.2 -Phive -Phive-thriftserver -DskipTests package
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 06:02 min
[INFO] Finished at: 2016-04-07T10:50:44-07:00
[INFO] Final Memory: 79M/853M
[INFO] ------------------------------------------------------------------------

The real root cause of this error occurs during Scala doc generation by doc-jar plugin. Is it possible to solve the real root cause by changing pom.xml to reorder the plugin execution stages or something else? I've already changed my another PR #12185 (DESC FUNCTION implementation) according this PR's approach, but it's a little too limited approach for me.

@dongjoon-hyun
Copy link
Member

Just for a record, here is the current error message from doc-jar. (The version of doc-jar is the latest.)

[INFO] <<< scala-maven-plugin:3.2.2:doc-jar (attach-scaladocs) < generate-sources @ spark-catalyst_2.11 <<<
[INFO] 
[INFO] --- scala-maven-plugin:3.2.2:doc-jar (attach-scaladocs) @ spark-catalyst_2.11 ---
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option PermSize=64m; support was removed in 8.0
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=512m; support was removed in 8.0
/Users/dongjoon/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:626: error: annotation argument needs to be a constant; found: "_FUNC_(str) - ".+("Returns str, with the first letter of each word in uppercase, all other letters in ").+("lowercase. Words are delimited by white space.")
    "Returns str, with the first letter of each word in uppercase, all other letters in " +
                                                                                          ^

@srowen
Copy link
Member

srowen commented Apr 7, 2016

I agree it's a bit of a strange problem. @dongjoon-hyun do you agree that this change is worth merging? it seems like it fixes the immediate symptom of a problem, even if we're not sure why the problem exists in the first place. It's not hacky so seems worth merging.

@dongjoon-hyun
Copy link
Member

Sure. I agree for merging.
The underlying problem will be investigated and resolved someday.

@jaceklaskowski
Copy link
Contributor Author

Please please merge it as soon as possible as I'm suffering from not having it in master every time I do the build :(

@srowen
Copy link
Member

srowen commented Apr 8, 2016

Merged to master

@asfgit asfgit closed this in 6447098 Apr 8, 2016
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.

5 participants