[GitHub] carbondata pull request #1443: [CARBONDATA-1524[CARBONDATA-1525] Added suppo...

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1443#discussion_r150170260
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/rdd/Compactor.scala ---
    @@ -89,23 +89,23 @@ object Compactor {
         }
     
         val mergeStatus =
    -    if (compactionType == CompactionType.IUD_UPDDEL_DELTA_COMPACTION) {
    -      new CarbonIUDMergerRDD(
    -        sc.sparkContext,
    -        new MergeResultImpl(),
    -        carbonLoadModel,
    -        carbonMergerMapping,
    -        execInstance
    -      ).collect
    -    } else {
    -      new CarbonMergerRDD(
    -        sc.sparkContext,
    -        new MergeResultImpl(),
    -        carbonLoadModel,
    -        carbonMergerMapping,
    -        execInstance
    -      ).collect
    -    }
    +      if (compactionType == CompactionType.IUD_UPDDEL_DELTA_COMPACTION) {
    --- End diff --
   
    don't format if you don't change the class


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1443#discussion_r150170350
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/execution/command/carbonTableSchemaCommon.scala ---
    @@ -33,8 +33,10 @@ import org.apache.carbondata.core.metadata.CarbonTableIdentifier
     import org.apache.carbondata.core.metadata.datatype.{DataType, DataTypes, DecimalType}
     import org.apache.carbondata.core.metadata.encoder.Encoding
     import org.apache.carbondata.core.metadata.schema._
    -import org.apache.carbondata.core.metadata.schema.table.{CarbonTable, RelationIdentifier, TableInfo, TableSchema}
    -import org.apache.carbondata.core.metadata.schema.table.column.{ColumnSchema, ParentColumnTableRelation}
    +import org.apache.carbondata.core.metadata.schema.table.{CarbonTable, RelationIdentifier,
    --- End diff --
   
    Don't change the class


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1443#discussion_r150170606
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/CarbonDropTableCommand.scala ---
    @@ -60,33 +62,40 @@ case class CarbonDropTableCommand(
             lock => carbonLocks += CarbonLockUtil.getLockObject(carbonTableIdentifier, lock)
           }
           LOGGER.audit(s"Deleting table [$tableName] under database [$dbName]")
    -
    -      // fires the event before dropping main table
    -      val carbonTable = CarbonMetadata.getInstance.getCarbonTable(dbName + "_" + tableName)
    +      val carbonTable: Option[CarbonTable] = try {
    --- End diff --
   
    try get from cache using `getTableFromMetadataCache` if not get then do lookup.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1443#discussion_r150170785
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/CarbonDropTableCommand.scala ---
    @@ -60,33 +62,40 @@ case class CarbonDropTableCommand(
             lock => carbonLocks += CarbonLockUtil.getLockObject(carbonTableIdentifier, lock)
           }
           LOGGER.audit(s"Deleting table [$tableName] under database [$dbName]")
    -
    -      // fires the event before dropping main table
    -      val carbonTable = CarbonMetadata.getInstance.getCarbonTable(dbName + "_" + tableName)
    +      val carbonTable: Option[CarbonTable] = try {
    +        Some(catalog.lookupRelation(identifier)(sparkSession)
    +          .asInstanceOf[CarbonRelation].metaData.carbonTable)
    +      } catch {
    +        case ex: NoSuchTableException =>
    +          if (!ifExistsSet) {
    +            throw ex
    +          }
    +          None
    +      }
           val operationContext = new OperationContext
           val dropTablePreEvent: DropTablePreEvent =
             DropTablePreEvent(
    -          carbonTable,
    +          carbonTable.get,
    --- End diff --
   
    you should not call get directly as in above catch there is chance of `None`


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/955/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1568/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/957/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1569/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1570/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/958/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1572/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1443#discussion_r150197463
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/CarbonDropTableCommand.scala ---
    @@ -60,33 +62,40 @@ case class CarbonDropTableCommand(
             lock => carbonLocks += CarbonLockUtil.getLockObject(carbonTableIdentifier, lock)
           }
           LOGGER.audit(s"Deleting table [$tableName] under database [$dbName]")
    -
    -      // fires the event before dropping main table
    -      val carbonTable = CarbonMetadata.getInstance.getCarbonTable(dbName + "_" + tableName)
    +      val carbonTable: Option[CarbonTable] = try {
    +        Some(catalog.lookupRelation(identifier)(sparkSession)
    +          .asInstanceOf[CarbonRelation].metaData.carbonTable)
    +      } catch {
    +        case ex: NoSuchTableException =>
    +          if (!ifExistsSet) {
    +            throw ex
    +          }
    +          None
    +      }
           val operationContext = new OperationContext
           val dropTablePreEvent: DropTablePreEvent =
             DropTablePreEvent(
    -          carbonTable,
    +          carbonTable.get,
    --- End diff --
   
    @fixed


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1573/



---

[qiuchenjian-2]

Github user kunal642 commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/965/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    LGTM


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/967/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1579/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/1004/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1443
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1621/



---