[GitHub] carbondata pull request #1425: [WIP]Thrift Changes to support Pre-aggregate

[qiuchenjian-2]

GitHub user kumarvishal09 opened a pull request:

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

    [WIP]Thrift Changes to support Pre-aggregate

    Thrift changes done to support Pre-Aggregate

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

    $ git pull https://github.com/kumarvishal09/incubator-carbondata master_AggSupportThriftChanges

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

    https://github.com/apache/carbondata/pull/1425.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 #1425
   
----
commit cc0210f70e2551bb031cb9854db0debdb3906e2d
Author: kumarvishal <[hidden email]>
Date:   2017-10-21T11:01:15Z

    Thrift Changes to support Pre-aggregate

----


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user kumarvishal09 commented on the issue:

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


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1425#discussion_r146163410
 
    --- Diff: format/src/main/thrift/schema.thrift ---
    @@ -165,9 +165,46 @@ struct TableSchema{
       4: optional map<string,string> tableProperties; // Table properties configured by the user
       5: optional BucketingInfo bucketingInfo; // Bucketing information
       6: optional PartitionInfo partitionInfo; // Partition information
    +  7: optional list<ColumnRelation> columnRelationList;
     }
     
    +struct RelationIdentifier {
    +   1: optional string databaseName;
    +   2: required string tableName;
    +   3: required string tableId;
    +}
    +
    +struct ColumnRelation{
    +   // child column id
    +   1: required string columnId;
    +   2: optional list<ParentColumnTableRelation> columnTableRelation;
    +}
    +
    +struct ParentColumnTableRelation {
    +   1: required RelationIdentifier relationInfo;
    +   2: required string columnId;
    +   3: required string columnName
    +}
    +
    +enum RelationType {
    +  AGGREGATION = 0;
    +}
    +struct RelationProperties {
    +  1: optional RelationType relationTypes;
    +  // to maintain properties like select query, query type like groupby, join
    +  2: optional map<string, string> properties;
    +}
    +
    +struct ChildSchema {
    --- End diff --
   
    Change as Datamap schema


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1425#discussion_r146163454
 
    --- Diff: format/src/main/thrift/schema.thrift ---
    @@ -165,9 +165,46 @@ struct TableSchema{
       4: optional map<string,string> tableProperties; // Table properties configured by the user
       5: optional BucketingInfo bucketingInfo; // Bucketing information
       6: optional PartitionInfo partitionInfo; // Partition information
    +  7: optional list<ColumnRelation> columnRelationList;
     }
     
    +struct RelationIdentifier {
    +   1: optional string databaseName;
    +   2: required string tableName;
    +   3: required string tableId;
    +}
    +
    +struct ColumnRelation{
    +   // child column id
    +   1: required string columnId;
    +   2: optional list<ParentColumnTableRelation> columnTableRelation;
    +}
    +
    +struct ParentColumnTableRelation {
    +   1: required RelationIdentifier relationInfo;
    +   2: required string columnId;
    +   3: required string columnName
    +}
    +
    +enum RelationType {
    +  AGGREGATION = 0;
    +}
    +struct RelationProperties {
    +  1: optional RelationType relationTypes;
    +  // to maintain properties like select query, query type like groupby, join
    +  2: optional map<string, string> properties;
    +}
    +
    +struct ChildSchema {
    +  1: required RelationIdentifier relationInfo;
    +  2: required TableSchema childTableSchema;
    --- End diff --
   
    Make it optional


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1425#discussion_r146163472
 
    --- Diff: format/src/main/thrift/schema.thrift ---
    @@ -165,9 +165,46 @@ struct TableSchema{
       4: optional map<string,string> tableProperties; // Table properties configured by the user
       5: optional BucketingInfo bucketingInfo; // Bucketing information
       6: optional PartitionInfo partitionInfo; // Partition information
    +  7: optional list<ColumnRelation> columnRelationList;
     }
     
    +struct RelationIdentifier {
    +   1: optional string databaseName;
    +   2: required string tableName;
    +   3: required string tableId;
    +}
    +
    +struct ColumnRelation{
    +   // child column id
    +   1: required string columnId;
    +   2: optional list<ParentColumnTableRelation> columnTableRelation;
    +}
    +
    +struct ParentColumnTableRelation {
    +   1: required RelationIdentifier relationInfo;
    +   2: required string columnId;
    +   3: required string columnName
    +}
    +
    +enum RelationType {
    +  AGGREGATION = 0;
    +}
    +struct RelationProperties {
    +  1: optional RelationType relationTypes;
    +  // to maintain properties like select query, query type like groupby, join
    +  2: optional map<string, string> properties;
    +}
    +
    +struct ChildSchema {
    +  1: required RelationIdentifier relationInfo;
    +  2: required TableSchema childTableSchema;
    +  3: optional RelationProperties relationProperties;
    +}
     struct TableInfo{
      1: required TableSchema fact_table;
      2: required list<TableSchema> aggregate_table_list;
    + 3: optional list<ChildSchema> childSchemas; // childSchema information
    + // only in case of child table it will be filled to maintain the relation with parent tables
    +   // to hanle operation like drop in that case parent table will be also updated
    +  4: optional list<RelationIdentifier> parentRelationInfo;
    --- End diff --
   
    not required again


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1425#discussion_r146163549
 
    --- Diff: format/src/main/thrift/schema.thrift ---
    @@ -165,9 +165,46 @@ struct TableSchema{
       4: optional map<string,string> tableProperties; // Table properties configured by the user
       5: optional BucketingInfo bucketingInfo; // Bucketing information
       6: optional PartitionInfo partitionInfo; // Partition information
    +  7: optional list<ColumnRelation> columnRelationList;
     }
     
    +struct RelationIdentifier {
    +   1: optional string databaseName;
    +   2: required string tableName;
    +   3: required string tableId;
    +}
    +
    +struct ColumnRelation{
    +   // child column id
    +   1: required string columnId;
    +   2: optional list<ParentColumnTableRelation> columnTableRelation;
    +}
    +
    +struct ParentColumnTableRelation {
    +   1: required RelationIdentifier relationInfo;
    +   2: required string columnId;
    +   3: required string columnName
    +}
    +
    +enum RelationType {
    +  AGGREGATION = 0;
    +}
    +struct RelationProperties {
    +  1: optional RelationType relationTypes;
    --- End diff --
   
    it is not required , but add the optional class name


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1425#discussion_r146163575
 
    --- Diff: format/src/main/thrift/schema.thrift ---
    @@ -165,9 +165,46 @@ struct TableSchema{
       4: optional map<string,string> tableProperties; // Table properties configured by the user
       5: optional BucketingInfo bucketingInfo; // Bucketing information
       6: optional PartitionInfo partitionInfo; // Partition information
    +  7: optional list<ColumnRelation> columnRelationList;
    --- End diff --
   
    move this to datamap schema


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1425
 
    just try this
    ```
   
    struct RelationIdentifier {
       1: optional string databaseName;
       2: required string tableName;
       3: required string tableId;
    }
   
    struct ColumnRelation{
       // child column
       1: required ColumnSchema table_column;
       2: optional list<ParentColumnTableRelation> columnTableRelation;
    }
   
    struct ParentColumnTableRelation {
       1: required RelationIdentifier relationInfo;
       2: required string columnId;
       3: required string columnName
    }
   
   
    struct DataMapSchema {
      1: required string className;
      2: optional list<RelationIdentifier> relationInfos;
      3: optional list<ColumnRelation> columnRelationList;
      // to maintain properties like select query, query type like groupby, join
      4: optional map<string, string> properties;
    }
   
    struct TableInfo{
      1: required TableSchema fact_table;
      2: required list<TableSchema> aggregate_table_list;
      3: optional list<DataMapSchema> dataMapSchemas; // dataMapSchema information
    }
    ```


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user kumarvishal09 commented on the issue:

    https://github.com/apache/carbondata/pull/1425
 
    @ravipesala Updated thrift.
    struct RelationIdentifier {
       1: optional string databaseName;
       2: required string tableName;
       3: required string tableId;
    }
   
    struct ParentColumnTableRelation {
       1: required RelationIdentifier relationIdentifier;
       2: required string columnId;
       3: required string columnName
    }
   
    struct DataMapSchema  {
        // class name
        1: required string className;
        // relation indentifier
        2: optional RelationIdentifier relationIdentifire;
        // in case of preaggregate it will be used to maintain the child schema
        // which will be usefull in case of query and data load
        3: optional TableSchema childTableSchema;
        // to maintain properties like select query, query type like groupby, join
        4: optional map<string, string> properties;
    }
   
    struct TableInfo{
    1: required TableSchema fact_table;
    2: required list<TableSchema> aggregate_table_list;
    3: optional list<DataMapSchema> dataMapSchemas; // childSchema information
    }
   
    Inside column schema added ParentColumnTableRelation


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1425#discussion_r146496476
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/memory/HeapMemoryAllocator.java ---
    @@ -17,12 +17,13 @@
     
     package org.apache.carbondata.core.memory;
     
    -import javax.annotation.concurrent.GuardedBy;
     import java.lang.ref.WeakReference;
     import java.util.HashMap;
     import java.util.LinkedList;
     import java.util.Map;
     
    +import javax.annotation.concurrent.GuardedBy;
    --- End diff --
   
    Not required to change


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1425#discussion_r146496730
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/converter/SchemaConverter.java ---
    @@ -100,4 +101,13 @@ TableSchema fromExternalToWrapperTableSchema(
        */
       TableInfo fromExternalToWrapperTableInfo(org.apache.carbondata.format.TableInfo externalTableInfo,
           String dbName, String tableName, String storePath);
    +
    +  /**
    +   * method to convert thrift datamap schema object to wrapper
    +   * data map object
    +   * @param thriftchildSchema
    +   * @return DataMapSchema
    +   */
    +  DataMapSchema fromExternalToWrapperChildSchema(
    --- End diff --
   
    change method name to reflect datamap in it


---