[GitHub] carbondata pull request #2141: Added comment for SDK writer API methods

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2141
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/3818/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2141#discussion_r181632323
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/executor/util/RestructureUtil.java ---
    @@ -337,11 +351,12 @@ public static Object getMeasureDefaultValueByType(ColumnSchema columnSchema,
        * @param blockExecutionInfo
        * @param queryMeasures        measures present in query
        * @param currentBlockMeasures current block measures
    +   * @param queryModel carbonQueryModel
        * @return measures present in the block
        */
       public static List<ProjectionMeasure> createMeasureInfoAndGetCurrentBlockQueryMeasures(
           BlockExecutionInfo blockExecutionInfo, List<ProjectionMeasure> queryMeasures,
    -      List<CarbonMeasure> currentBlockMeasures) {
    +      List<CarbonMeasure> currentBlockMeasures, QueryModel queryModel) {
    --- End diff --
   
    Pass flag, dont pass all the querymodel, Restructuring doesnot have any relation with Querymodel


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2141#discussion_r181632750
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/executor/util/RestructureUtil.java ---
    @@ -354,7 +369,13 @@ public static Object getMeasureDefaultValueByType(ColumnSchema columnSchema,
           // then setting measure exists is true
           // otherwise adding a default value of a measure
           for (CarbonMeasure carbonMeasure : currentBlockMeasures) {
    -        if (carbonMeasure.getColumnId().equals(queryMeasure.getMeasure().getColumnId())) {
    +        // If it is unmanaged table just check the column names, no need to validate column id as
    +        // multiple sdk's output placed in a single folder doesn't have same column ID but can
    +        // have same column name
    +        if (carbonMeasure.getColumnId().equals(queryMeasure.getMeasure().getColumnId()) ||
    +            ((queryModel != null) && (queryModel.getTable().getTableInfo().isUnManagedTable()) &&
    --- End diff --
   
    remove null check and add method isColumnMatches


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2141#discussion_r181634024
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/createTable/TestUnmanagedCarbonTable.scala ---
    @@ -266,16 +270,23 @@ class TestUnmanagedCarbonTable extends QueryTest with BeforeAndAfterAll {
           .contains("Unsupported operation on unmanaged table"))
     
         //12. Streaming table creation
    -    // External table don't accept table properties
    +    // No need as External table don't accept table properties
    +
    +    //13. Alter table rename command
    +    exception = intercept[MalformedCarbonCommandException] {
    +      sql("ALTER TABLE sdkOutputTable RENAME to newTable")
    +    }
    +    assert(exception.getMessage()
    +      .contains("Unsupported operation on unmanaged table"))
     
         sql("DROP TABLE sdkOutputTable")
    -    // drop table should not delete the files
    +     //drop table should not delete the files
    --- End diff --
   
    correct indent


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2141#discussion_r181634921
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableRenameCommand.scala ---
    @@ -73,22 +73,27 @@ private[sql] case class CarbonAlterTableRenameCommand(
                        s"Table $oldDatabaseName.$oldTableName does not exist")
           throwMetadataException(oldDatabaseName, oldTableName, "Table does not exist")
         }
    +
    +    var carbonTable: CarbonTable = null
    +    carbonTable = metastore.lookupRelation(Some(oldDatabaseName), oldTableName)(sparkSession)
    --- End diff --
   
    This cannot be moved outside lock


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2141#discussion_r181637617
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/CarbonFileMetastore.scala ---
    @@ -565,7 +563,9 @@ class CarbonFileMetastore extends CarbonMetaStore {
                 tableModifiedTimeStore.get(CarbonCommonConstants.DATABASE_DEFAULT_NAME))) {
             metadata.carbonTables = metadata.carbonTables.filterNot(
               table => table.getTableName.equalsIgnoreCase(tableIdentifier.table) &&
    -        table.getDatabaseName.equalsIgnoreCase(tableIdentifier.database.getOrElse("default")))
    +                   table.getDatabaseName.equalsIgnoreCase(
    --- End diff --
   
    Why it coming here for unmanged table


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2141#discussion_r181646540
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/CarbonFileMetastore.scala ---
    @@ -565,7 +563,9 @@ class CarbonFileMetastore extends CarbonMetaStore {
                 tableModifiedTimeStore.get(CarbonCommonConstants.DATABASE_DEFAULT_NAME))) {
             metadata.carbonTables = metadata.carbonTables.filterNot(
               table => table.getTableName.equalsIgnoreCase(tableIdentifier.table) &&
    -        table.getDatabaseName.equalsIgnoreCase(tableIdentifier.database.getOrElse("default")))
    +                   table.getDatabaseName.equalsIgnoreCase(
    --- End diff --
   
    Ran all the test case. It is not coming now. Removed this changes


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2141#discussion_r181646642
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableRenameCommand.scala ---
    @@ -73,22 +73,27 @@ private[sql] case class CarbonAlterTableRenameCommand(
                        s"Table $oldDatabaseName.$oldTableName does not exist")
           throwMetadataException(oldDatabaseName, oldTableName, "Table does not exist")
         }
    +
    +    var carbonTable: CarbonTable = null
    +    carbonTable = metastore.lookupRelation(Some(oldDatabaseName), oldTableName)(sparkSession)
    --- End diff --
   
    OK. modified


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2141#discussion_r181647929
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/createTable/TestUnmanagedCarbonTable.scala ---
    @@ -266,16 +270,23 @@ class TestUnmanagedCarbonTable extends QueryTest with BeforeAndAfterAll {
           .contains("Unsupported operation on unmanaged table"))
     
         //12. Streaming table creation
    -    // External table don't accept table properties
    +    // No need as External table don't accept table properties
    +
    +    //13. Alter table rename command
    +    exception = intercept[MalformedCarbonCommandException] {
    +      sql("ALTER TABLE sdkOutputTable RENAME to newTable")
    +    }
    +    assert(exception.getMessage()
    +      .contains("Unsupported operation on unmanaged table"))
     
         sql("DROP TABLE sdkOutputTable")
    -    // drop table should not delete the files
    +     //drop table should not delete the files
    --- End diff --
   
    done


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2141#discussion_r181648541
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/executor/util/RestructureUtil.java ---
    @@ -337,11 +351,12 @@ public static Object getMeasureDefaultValueByType(ColumnSchema columnSchema,
        * @param blockExecutionInfo
        * @param queryMeasures        measures present in query
        * @param currentBlockMeasures current block measures
    +   * @param queryModel carbonQueryModel
        * @return measures present in the block
        */
       public static List<ProjectionMeasure> createMeasureInfoAndGetCurrentBlockQueryMeasures(
           BlockExecutionInfo blockExecutionInfo, List<ProjectionMeasure> queryMeasures,
    -      List<CarbonMeasure> currentBlockMeasures) {
    +      List<CarbonMeasure> currentBlockMeasures, QueryModel queryModel) {
    --- End diff --
   
    OK


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2141#discussion_r181648562
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/executor/util/RestructureUtil.java ---
    @@ -354,7 +369,13 @@ public static Object getMeasureDefaultValueByType(ColumnSchema columnSchema,
           // then setting measure exists is true
           // otherwise adding a default value of a measure
           for (CarbonMeasure carbonMeasure : currentBlockMeasures) {
    -        if (carbonMeasure.getColumnId().equals(queryMeasure.getMeasure().getColumnId())) {
    +        // If it is unmanaged table just check the column names, no need to validate column id as
    +        // multiple sdk's output placed in a single folder doesn't have same column ID but can
    +        // have same column name
    +        if (carbonMeasure.getColumnId().equals(queryMeasure.getMeasure().getColumnId()) ||
    +            ((queryModel != null) && (queryModel.getTable().getTableInfo().isUnManagedTable()) &&
    --- End diff --
   
    ok


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2141#discussion_r181650233
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/executor/util/RestructureUtil.java ---
    @@ -354,7 +369,13 @@ public static Object getMeasureDefaultValueByType(ColumnSchema columnSchema,
           // then setting measure exists is true
           // otherwise adding a default value of a measure
           for (CarbonMeasure carbonMeasure : currentBlockMeasures) {
    -        if (carbonMeasure.getColumnId().equals(queryMeasure.getMeasure().getColumnId())) {
    +        // If it is unmanaged table just check the column names, no need to validate column id as
    +        // multiple sdk's output placed in a single folder doesn't have same column ID but can
    +        // have same column name
    +        if (carbonMeasure.getColumnId().equals(queryMeasure.getMeasure().getColumnId()) ||
    +            ((queryModel != null) && (queryModel.getTable().getTableInfo().isUnManagedTable()) &&
    --- End diff --
   
    removed null check. cannot call from a method as one is for measure type and other is a dimension type.


---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

    https://github.com/apache/carbondata/pull/2141
 
    Retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---