[GitHub] carbondata pull request #2961: Fixing the getOrCreateCarbonSession method pa...

[qiuchenjian-2]

GitHub user zygitup opened a pull request:

    https://github.com/apache/carbondata/pull/2961

    Fixing the getOrCreateCarbonSession method parameter to an empty stri…[[JIRA CARBONDATA-3119]]

    1.No interface changes
   
    2.Backward compatibility is not affected
   
    3.The document does not need to be updated
   
    val carbon = SparkSession.builder().config(sc.getConf).getOrCreateCarbonSession("")
    val carbon = SparkSession.builder().config(sc.getConf).getOrCreateCarbonSession(" ")
    carbon.sql("SELECT * FROM TABLE").show() is ok.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/zygitup/carbondata master

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/2961.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2961
   
----
commit f934d7938ae3694f2508b1c5c278c805892ba6ec
Author: zygitup <18310672430@...>
Date:   2018-11-28T12:38:56Z

    Fixing the getOrCreateCarbonSession method parameter to an empty string causes the select table data to be empty

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2961
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user qiuchenjian commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2961#discussion_r237095448
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSession.scala ---
    @@ -248,7 +248,7 @@ object CarbonSession {
     
             session = new CarbonSession(sparkContext, None, !enableInMemCatlog)
             val carbonProperties = CarbonProperties.getInstance()
    -        if (storePath != null) {
    +        if (storePath != null && !storePath.trim.isEmpty) {
    --- End diff --
   
    why not use StringUtils or other String tools


---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/carbondata/pull/2961
 
    add to whitelist


---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/carbondata/pull/2961
 
    the pr title is not correct, the format should be : [JIRA NUMBER] PR description


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2961
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1574/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2961
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1786/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2961
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9833/



---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

    https://github.com/apache/carbondata/pull/2961
 
    Please optimize the title, for example: [CARBONDATA-3119]Fix the error of getOrCreateCarbonSession method parameter is an empty string
    Please optimize the PR content, keep the checklist format and finish the checklist


---

[qiuchenjian-2]

Github user xubo245 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2961#discussion_r237334369
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSession.scala ---
    @@ -248,7 +248,7 @@ object CarbonSession {
     
             session = new CarbonSession(sparkContext, None, !enableInMemCatlog)
             val carbonProperties = CarbonProperties.getInstance()
    -        if (storePath != null) {
    +        if (storePath != null && !storePath.trim.isEmpty) {
    --- End diff --
   
    Which method? Can you give details?


---

[qiuchenjian-2]

Github user qiuchenjian commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2961#discussion_r237335152
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSession.scala ---
    @@ -248,7 +248,7 @@ object CarbonSession {
     
             session = new CarbonSession(sparkContext, None, !enableInMemCatlog)
             val carbonProperties = CarbonProperties.getInstance()
    -        if (storePath != null) {
    +        if (storePath != null && !storePath.trim.isEmpty) {
    --- End diff --
   
    such as StringUtils.isNotEmpty


---

[qiuchenjian-2]

Github user xubo245 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2961#discussion_r237345420
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSession.scala ---
    @@ -248,7 +248,7 @@ object CarbonSession {
     
             session = new CarbonSession(sparkContext, None, !enableInMemCatlog)
             val carbonProperties = CarbonProperties.getInstance()
    -        if (storePath != null) {
    +        if (storePath != null && !storePath.trim.isEmpty) {
    --- End diff --
   
    StringUtils.isNotEmpty shouldn't support " "


---

[qiuchenjian-2]

Github user xubo245 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2961#discussion_r237722028
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSession.scala ---
    @@ -180,7 +180,7 @@ object CarbonSession {
           val userSuppliedContext: Option[SparkContext] =
             getValue("userSuppliedContext", builder).asInstanceOf[Option[SparkContext]]
     
    -      if (metaStorePath != null) {
    +      if (metaStorePath != null && !metaStorePath.trim.isEmpty) {
    --- End diff --
   
    Can you add some test case to test it?


---

[qiuchenjian-2]

Github user xubo245 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2961#discussion_r237722035
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSession.scala ---
    @@ -248,7 +248,7 @@ object CarbonSession {
     
             session = new CarbonSession(sparkContext, None, !enableInMemCatlog)
             val carbonProperties = CarbonProperties.getInstance()
    -        if (storePath != null) {
    +        if (storePath != null && !storePath.trim.isEmpty) {
    --- End diff --
   
    Can you add some test case to test it?


---

[qiuchenjian-2]

Github user zzcclp commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2961#discussion_r237722775
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSession.scala ---
    @@ -180,7 +180,7 @@ object CarbonSession {
           val userSuppliedContext: Option[SparkContext] =
             getValue("userSuppliedContext", builder).asInstanceOf[Option[SparkContext]]
     
    -      if (metaStorePath != null) {
    +      if (metaStorePath != null && !metaStorePath.trim.isEmpty) {
    --- End diff --
   
    Use StringUtils.isNotBlank.


---

[qiuchenjian-2]

Github user zzcclp commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2961#discussion_r237722816
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSession.scala ---
    @@ -248,7 +248,7 @@ object CarbonSession {
     
             session = new CarbonSession(sparkContext, None, !enableInMemCatlog)
             val carbonProperties = CarbonProperties.getInstance()
    -        if (storePath != null) {
    +        if (storePath != null && !storePath.trim.isEmpty) {
    --- End diff --
   
    Use StringUtils.isNotBlank.


---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2961
 
    +1 for @zzcclp 's comments


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2961
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1595/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2961
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9855/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2961
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1807/



---