[GitHub] incubator-carbondata pull request #816: [CARBONDATA-944] Fixed unwanted exce...

[qiuchenjian-2]

GitHub user bhavya411 opened a pull request:

    https://github.com/apache/incubator-carbondata/pull/816

    [CARBONDATA-944] Fixed unwanted exception in case of Drop table through spark-sql-CLI

    This PR includes a fix for Table Already Exists exception in case of Drop Table command.
   
    Build verification with spark-1.6 successful
    Build with spark-2.1 successful
    Verified The defect Manually
   
                     
    ---


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

    $ git pull https://github.com/bhavya411/incubator-carbondata CARBONDATA-944

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

    https://github.com/apache/incubator-carbondata/pull/816.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 #816
   
----
commit 7c4ca35681ece52d674120d668c876349caa9131
Author: Bhavya <[hidden email]>
Date:   2017-04-19T07:20:54Z

    Fixed unwanted exception in case of Drop table through spark-sql-CLI

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/816
 
    Build Success with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1694/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/incubator-carbondata/pull/816#discussion_r112972168
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSource.scala ---
    @@ -184,4 +194,29 @@ class CarbonSource extends CreatableRelationProvider with RelationProvider
         }
       }
     
    +  /**
    +   * Returns the path of the table
    +   * @param sparkSession
    +   * @param dbName
    +   * @param tableName
    +   * @return
    +   */
    +  private def getPathForTable(sparkSession: SparkSession, dbName: String,
    +      tableName : String): String = {
    +
    +    if (StringUtils.isBlank(tableName)) {
    +      throw new MalformedCarbonCommandException("The Specified Table Name is Blank")
    +    }
    +    if (tableName.contains(" ")) {
    +      throw new MalformedCarbonCommandException("Table Name Should not have spaces ")
    +    }
    +    try {
    +      CarbonEnv.get.carbonMetastore.lookupRelation(Option(dbName), tableName)(sparkSession)
    --- End diff --
   
    Why not using the below code , keep consistent with createTableIfNotExists?
   
    CarbonEnv.getInstance(sparkSession).carbonMetastore
            .lookupRelation(Option(dbName), tableName)(sparkSession)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/incubator-carbondata/pull/816#discussion_r112972641
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSource.scala ---
    @@ -184,4 +194,29 @@ class CarbonSource extends CreatableRelationProvider with RelationProvider
         }
       }
     
    +  /**
    +   * Returns the path of the table
    +   * @param sparkSession
    +   * @param dbName
    +   * @param tableName
    +   * @return
    +   */
    +  private def getPathForTable(sparkSession: SparkSession, dbName: String,
    +      tableName : String): String = {
    +
    +    if (StringUtils.isBlank(tableName)) {
    +      throw new MalformedCarbonCommandException("The Specified Table Name is Blank")
    +    }
    +    if (tableName.contains(" ")) {
    +      throw new MalformedCarbonCommandException("Table Name Should not have spaces ")
    +    }
    +    try {
    +      CarbonEnv.get.carbonMetastore.lookupRelation(Option(dbName), tableName)(sparkSession)
    +      CarbonEnv.get.carbonMetastore.storePath + s"/$dbName/$tableName"
    --- End diff --
   
    CarbonEnv.getInstance(sparkSession).carbonMetastore.storePath + s"/$dbName/$tableName"


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/incubator-carbondata/pull/816#discussion_r112974468
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSource.scala ---
    @@ -184,4 +194,29 @@ class CarbonSource extends CreatableRelationProvider with RelationProvider
         }
       }
     
    +  /**
    +   * Returns the path of the table
    +   * @param sparkSession
    +   * @param dbName
    +   * @param tableName
    +   * @return
    +   */
    +  private def getPathForTable(sparkSession: SparkSession, dbName: String,
    +      tableName : String): String = {
    +
    +    if (StringUtils.isBlank(tableName)) {
    +      throw new MalformedCarbonCommandException("The Specified Table Name is Blank")
    +    }
    +    if (tableName.contains(" ")) {
    +      throw new MalformedCarbonCommandException("Table Name Should not have spaces ")
    +    }
    +    try {
    +      CarbonEnv.get.carbonMetastore.lookupRelation(Option(dbName), tableName)(sparkSession)
    +      CarbonEnv.get.carbonMetastore.storePath + s"/$dbName/$tableName"
    +    } catch {
    +      case ex: Exception =>
    +        throw new Exception(s"do not have $dbName and $tableName for carbon table", ex)
    --- End diff --
   
    How about :
    throw new Exception(s"Do not have $dbName and $tableName", ex)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/816
 
    Build Success with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1788/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/incubator-carbondata/pull/816#discussion_r113853461
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSource.scala ---
    @@ -112,7 +112,17 @@ class CarbonSource extends CreatableRelationProvider with RelationProvider
           dataSchema: StructType): BaseRelation = {
         CarbonEnv.getInstance(sqlContext.sparkSession)
         addLateDecodeOptimization(sqlContext.sparkSession)
    -    val path = createTableIfNotExists(sqlContext.sparkSession, parameters, dataSchema)
    +    val dbName: String = parameters.getOrElse("dbName",
    +      CarbonCommonConstants.DATABASE_DEFAULT_NAME).toLowerCase
    +    val tableName: String = parameters.getOrElse("tableName", "default_table").toLowerCase
    --- End diff --
   
    can you do this check in caller function where you can use CarbonOption to get the dbName and tableName instead of doing it here?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/incubator-carbondata/pull/816#discussion_r114288845
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSource.scala ---
    @@ -112,7 +112,17 @@ class CarbonSource extends CreatableRelationProvider with RelationProvider
           dataSchema: StructType): BaseRelation = {
         CarbonEnv.getInstance(sqlContext.sparkSession)
         addLateDecodeOptimization(sqlContext.sparkSession)
    -    val path = createTableIfNotExists(sqlContext.sparkSession, parameters, dataSchema)
    +    val dbName: String = parameters.getOrElse("dbName",
    +      CarbonCommonConstants.DATABASE_DEFAULT_NAME).toLowerCase
    +    val tableName: String = parameters.getOrElse("tableName", "default_table").toLowerCase
    --- End diff --
   
    @jackylk  I can not call it in the caller function as the createRelation method is a framework metthod so it can be called while Using clause is used , if I move the code to caller function the createTableIfnotExists will not be called in that scenario. Please let me know if we still need to do it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/816
 
    LGTM


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-carbondata/pull/816


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---