[CARBONDATA-584]added validation for table is not empty #511

anubhav100 · 2017-01-09T15:59:51Z

when using either the carbon session or spark source it allows to create bucketed table even if table name in options is empty in this i am correcting this bug by validating table name

CarbonDataQA · 2017-01-09T16:08:25Z

Build Success with Spark 1.5.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/526/

jackylk · 2017-01-09T23:39:35Z

Please provide description to this PR. Are you using CarbonSession or SparkSession? How to reproduce this bug?

CarbonDataQA · 2017-01-10T06:28:19Z

Build Success with Spark 1.5.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/531/

jackylk · 2017-01-12T00:14:14Z

integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSource.scala

@@ -108,6 +111,9 @@ class CarbonSource extends CreatableRelationProvider

    val dbName: String = parameters.getOrElse("dbName", CarbonCommonConstants.DATABASE_DEFAULT_NAME)
    val tableName: String = parameters.getOrElse("tableName", "default_table")
+    if(tableName.isEmpty || tableName.contains("")) {


I think you can use StringUtils.isBlank utility function

anubhav100 · 2017-01-13T04:50:59Z

@jackylk sure done

CarbonDataQA · 2017-01-13T04:58:30Z

Build Success with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/575/

jackylk · 2017-01-13T14:35:42Z

integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSource.scala

@@ -108,6 +110,10 @@ class CarbonSource extends CreatableRelationProvider

    val dbName: String = parameters.getOrElse("dbName", CarbonCommonConstants.DATABASE_DEFAULT_NAME)
    val tableName: String = parameters.getOrElse("tableName", "default_table")
+    if(org.apache.commons.lang.StringUtils.isBlank(tableName) ||


add space after if
import this class at the beginning of file

jackylk · 2017-01-13T14:36:26Z

integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSource.scala

@@ -108,6 +110,10 @@ class CarbonSource extends CreatableRelationProvider

    val dbName: String = parameters.getOrElse("dbName", CarbonCommonConstants.DATABASE_DEFAULT_NAME)
    val tableName: String = parameters.getOrElse("tableName", "default_table")
+    if(org.apache.commons.lang.StringUtils.isBlank(tableName) ||
+       org.apache.commons.lang.StringUtils.isWhitespace(tableName)) {


I think isBlank will check whether it is space, so this is not required

@jackylk but what if table name is "a n u b h a v" i think it will not check for its validation

In this case, it will fail in parser. It will not come to here

jackylk · 2017-01-13T14:36:59Z

integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSource.scala

@@ -108,6 +110,10 @@ class CarbonSource extends CreatableRelationProvider

    val dbName: String = parameters.getOrElse("dbName", CarbonCommonConstants.DATABASE_DEFAULT_NAME)
    val tableName: String = parameters.getOrElse("tableName", "default_table")
+    if(org.apache.commons.lang.StringUtils.isBlank(tableName) ||
+       org.apache.commons.lang.StringUtils.isWhitespace(tableName)) {
+      throw new MalformedCarbonCommandException("INVALID TABLE NAME")


You can say it is empty table name

@jackylk sure i am correcting it

jackylk · 2017-01-13T14:38:08Z

integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSource.scala

@@ -108,6 +110,10 @@ class CarbonSource extends CreatableRelationProvider

    val dbName: String = parameters.getOrElse("dbName", CarbonCommonConstants.DATABASE_DEFAULT_NAME)
    val tableName: String = parameters.getOrElse("tableName", "default_table")
+    if(org.apache.commons.lang.StringUtils.isBlank(tableName) ||
+       org.apache.commons.lang.StringUtils.isWhitespace(tableName)) {
+      throw new MalformedCarbonCommandException("INVALID TABLE NAME")


You can say it is empty table name

@jackylk i think emptyTableName will make less sense because it is not gurranted that it will be empty

if the if check is true, it means the table name passing by user is blank

I think there is misunderstanding, I suggest to give "the specified table name is blank" in the exception message. Not suggesting to change the variable name.

CarbonDataQA · 2017-01-13T17:34:21Z

Build Success with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/593/

CarbonDataQA · 2017-01-13T18:00:23Z

Build Success with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/594/

anubhav100 · 2017-01-13T18:02:21Z

@jackylk all issues are resolved

CarbonDataQA · 2017-01-13T18:20:23Z

Build Success with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/595/

jackylk · 2017-01-14T04:28:40Z

integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSource.scala

-    val tableName: String = parameters.getOrElse("tableName", "default_table")
+    val emptyTableName: String = parameters.getOrElse("tableName", "default_table")
+    if (StringUtils.isBlank(emptyTableName)) {
+      throw new MalformedCarbonCommandException("INVALID TABLE NAME")


I think there is misunderstanding, I suggest to give "the specified table name is blank" in the exception message. Not suggesting to change the variable name.

@jackylk oh get that now thats why i am asking you i m correcting it now

@jackylk corrected it sry for mistake

CarbonDataQA · 2017-01-14T06:38:28Z

Build Success with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/599/

jackylk · 2017-01-14T07:28:08Z

LGTM
Thanks for working on this

* Inital commit * Remove unused patch

added validation for table is not empty and don not contain any spaces

05bd1ea

reformatted the code

da7714a

jackylk reviewed Jan 12, 2017

View reviewed changes

replace validation with string utils

b288314

removed java style error

e731b53

jackylk reviewed Jan 13, 2017

View reviewed changes

removed unnecesary code

d43393f

removed java style err

f987069

removed unused import

c215739

jackylk reviewed Jan 14, 2017

View reviewed changes

changed the exception message

134856f

asfgit closed this in 392bc29 Jan 14, 2017

Beyyes pushed a commit to Beyyes/carbondata that referenced this pull request Jul 12, 2018

VC configratuion update. Change the queue limit to 100% (apache#511)

377328a

* Inital commit * Remove unused patch

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CARBONDATA-584]added validation for table is not empty #511

[CARBONDATA-584]added validation for table is not empty #511

anubhav100 commented Jan 9, 2017 •

edited

Loading

CarbonDataQA commented Jan 9, 2017

jackylk commented Jan 9, 2017

CarbonDataQA commented Jan 10, 2017

jackylk Jan 12, 2017

anubhav100 commented Jan 13, 2017

CarbonDataQA commented Jan 13, 2017

jackylk Jan 13, 2017

jackylk Jan 13, 2017

anubhav100 Jan 13, 2017

jackylk Jan 13, 2017

jackylk Jan 13, 2017

anubhav100 Jan 13, 2017

jackylk Jan 13, 2017

anubhav100 Jan 13, 2017

jackylk Jan 13, 2017

jackylk Jan 14, 2017 •

edited

Loading

CarbonDataQA commented Jan 13, 2017

CarbonDataQA commented Jan 13, 2017

anubhav100 commented Jan 13, 2017

CarbonDataQA commented Jan 13, 2017

jackylk Jan 14, 2017

anubhav100 Jan 14, 2017

anubhav100 Jan 14, 2017

CarbonDataQA commented Jan 14, 2017

jackylk commented Jan 14, 2017

[CARBONDATA-584]added validation for table is not empty #511

[CARBONDATA-584]added validation for table is not empty #511

Conversation

anubhav100 commented Jan 9, 2017 • edited Loading

CarbonDataQA commented Jan 9, 2017

jackylk commented Jan 9, 2017

CarbonDataQA commented Jan 10, 2017

Choose a reason for hiding this comment

anubhav100 commented Jan 13, 2017

CarbonDataQA commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackylk Jan 14, 2017 • edited Loading

Choose a reason for hiding this comment

CarbonDataQA commented Jan 13, 2017

CarbonDataQA commented Jan 13, 2017

anubhav100 commented Jan 13, 2017

CarbonDataQA commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA commented Jan 14, 2017

jackylk commented Jan 14, 2017

anubhav100 commented Jan 9, 2017 •

edited

Loading

jackylk Jan 14, 2017 •

edited

Loading