[GitHub] carbondata pull request #2791: [HOTFIX]correct the exception handling in loo...

[qiuchenjian-2]

GitHub user akashrn5 opened a pull request:

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

    [HOTFIX]correct the exception handling in lookup relation

    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     
     - [ ] Any backward compatibility impacted?
     
     - [ ] Document update required?
   
     - [ ] Testing done
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
            - How it is tested? Please attach test report.
            - Is it a performance related change? Please attach the performance test report.
            - Any additional information to help reviewers in testing this change.
           
     - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
   


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

    $ git pull https://github.com/akashrn5/incubator-carbondata exception

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

    https://github.com/apache/carbondata/pull/2791.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 #2791
   
----
commit c4b252e30bcb758adbe4192f886983cf44bcae75
Author: akashrn5 <akashnilugal@...>
Date:   2018-09-29T10:01:21Z

    correct the exception handling in lookup relation

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2791#discussion_r221447092
 
    --- Diff: integration/spark2/src/main/commonTo2.2And2.3/org/apache/spark/sql/hive/CarbonInMemorySessionState.scala ---
    @@ -18,6 +18,7 @@ package org.apache.spark.sql.hive
     
     import org.apache.hadoop.conf.Configuration
     import org.apache.hadoop.fs.Path
    +import org.apache.hadoop.hive.ql.metadata.HiveException
    --- End diff --
   
    there shall be a space after hadoop module imports right? how this is passing scala checkstyle? we dont have such rule? can you please check once


---

[qiuchenjian-2]

Github user sujith71955 commented on the issue:

    https://github.com/apache/carbondata/pull/2791
 
    @akashrn5  It will be very helpful if you can provide proper descriptions for the PR.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2791#discussion_r221447314
 
    --- Diff: integration/spark2/src/main/commonTo2.2And2.3/org/apache/spark/sql/hive/CarbonInMemorySessionState.scala ---
    @@ -157,7 +158,13 @@ class InMemorySessionCatalog(
       }
     
       override def lookupRelation(name: TableIdentifier): LogicalPlan = {
    -    val rtnRelation = super.lookupRelation(name)
    +    val rtnRelation: LogicalPlan =
    +      try {
    --- End diff --
   
    why we need this try/catch, its super class doesn't throw any exception right, moreover we are throwing out the same exception object, can you please define the intention of this statement?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2791#discussion_r221448493
 
    --- Diff: integration/spark2/src/main/commonTo2.2And2.3/org/apache/spark/sql/hive/CarbonInMemorySessionState.scala ---
    @@ -18,6 +18,7 @@ package org.apache.spark.sql.hive
     
     import org.apache.hadoop.conf.Configuration
     import org.apache.hadoop.fs.Path
    +import org.apache.hadoop.hive.ql.metadata.HiveException
    --- End diff --
   
    scalastyle is passing for me in local too, initially also there was no space


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2791#discussion_r221448520
 
    --- Diff: integration/spark2/src/main/commonTo2.2And2.3/org/apache/spark/sql/hive/CarbonInMemorySessionState.scala ---
    @@ -157,7 +158,13 @@ class InMemorySessionCatalog(
       }
     
       override def lookupRelation(name: TableIdentifier): LogicalPlan = {
    -    val rtnRelation = super.lookupRelation(name)
    +    val rtnRelation: LogicalPlan =
    +      try {
    --- End diff --
   
    i have described in description section, may be that will explain, can you please have a look?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2791#discussion_r221448768
 
    --- Diff: integration/spark2/src/main/commonTo2.2And2.3/org/apache/spark/sql/hive/CarbonInMemorySessionState.scala ---
    @@ -18,6 +18,7 @@ package org.apache.spark.sql.hive
     
     import org.apache.hadoop.conf.Configuration
     import org.apache.hadoop.fs.Path
    +import org.apache.hadoop.hive.ql.metadata.HiveException
    --- End diff --
   
    I think there should be space between different system imports, like there shall be space between carbon and spark system, same shall be followed in case of hadoop also, spark is following the same styling format. please have a look, i think you can revisit the scala style guideline once again and update if required.
    



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2791#discussion_r221449001
 
    --- Diff: integration/spark2/src/main/commonTo2.2And2.3/org/apache/spark/sql/hive/CarbonInMemorySessionState.scala ---
    @@ -157,7 +158,13 @@ class InMemorySessionCatalog(
       }
     
       override def lookupRelation(name: TableIdentifier): LogicalPlan = {
    -    val rtnRelation = super.lookupRelation(name)
    +    val rtnRelation: LogicalPlan =
    +      try {
    +        super.lookupRelation(name)
    +      } catch {
    +        case ex: Exception =>
    +          throw ex
    --- End diff --
   
    but here you are returning same type of exception object right? or am i missing anything?what about wraping this exception object with custom exception object ? or else i cannot see anything in this statement you are doing different


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2791#discussion_r221449108
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/CarbonFileMetastore.scala ---
    @@ -208,7 +209,10 @@ class CarbonFileMetastore extends CarbonMetaStore {
         try {
           lookupRelation(tableIdentifier)(sparkSession)
         } catch {
    -      case _: Exception =>
    +      case ex: Exception =>
    +        if (ex.getCause.isInstanceOf[HiveException]) {
    +          throw ex
    +        }
    --- End diff --
   
    i suggest if you are  skipping the exception propagation, better you log the reason why you are returning false.
    this will be better for a secondary developer to locate the exact problem from a log  in some unwanted failure scenarios.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2791#discussion_r221449294
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/CarbonFileMetastore.scala ---
    @@ -208,7 +209,10 @@ class CarbonFileMetastore extends CarbonMetaStore {
         try {
           lookupRelation(tableIdentifier)(sparkSession)
         } catch {
    -      case _: Exception =>
    +      case ex: Exception =>
    +        if (ex.getCause.isInstanceOf[HiveException]) {
    +          throw ex
    +        }
    --- End diff --
   
    I think it will better to have a testcases here? i can see from your code in CarbonFileMetastore.scala  that you are only expecting a HiveException type to be thrown out, other exceptions you are ignoring.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2791#discussion_r222252487
 
    --- Diff: integration/spark2/src/main/spark2.1/org/apache/spark/sql/hive/CarbonSessionState.scala ---
    @@ -141,7 +141,13 @@ class CarbonHiveSessionCatalog(
        */
       override def lookupRelation(name: TableIdentifier,
           alias: Option[String]): LogicalPlan = {
    -    val rtnRelation = super.lookupRelation(name, alias)
    +    val rtnRelation =
    +      try {
    +        super.lookupRelation(name, alias)
    +      } catch {
    +        case ex: Exception =>
    --- End diff --
   
    This change is not required. Catch and throwing same exception.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2791#discussion_r222409727
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/CarbonFileMetastore.scala ---
    @@ -208,7 +209,10 @@ class CarbonFileMetastore extends CarbonMetaStore {
         try {
           lookupRelation(tableIdentifier)(sparkSession)
         } catch {
    -      case _: Exception =>
    +      case ex: Exception =>
    +        if (ex.getCause.isInstanceOf[HiveException]) {
    +          throw ex
    +        }
    --- End diff --
   
    i checked the flow and permission errors will be thrown by Hive as HiveException and wrapped as analysis exception, and all the other exceptions will be same as i have described in description, during that time we cannot just catch exception and return false as table not exists. so i m checking as HiveException


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2791#discussion_r222415572
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/CarbonFileMetastore.scala ---
    @@ -208,7 +209,10 @@ class CarbonFileMetastore extends CarbonMetaStore {
         try {
           lookupRelation(tableIdentifier)(sparkSession)
         } catch {
    -      case _: Exception =>
    +      case ex: Exception =>
    +        if (ex.getCause.isInstanceOf[HiveException]) {
    +          throw ex
    +        }
    --- End diff --
   
    Didn't got your context of explanation is  for which review comment?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2791#discussion_r222416107
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/CarbonFileMetastore.scala ---
    @@ -23,6 +23,7 @@ import java.net.URI
     import scala.collection.mutable.ArrayBuffer
     
     import org.apache.hadoop.fs.permission.{FsAction, FsPermission}
    +import org.apache.hadoop.hive.ql.metadata.HiveException
    --- End diff --
   
    nit: space is required, please check the rule


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2791
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/888/



---