[GitHub] carbondata pull request #1919: [WIP]Refactor datamap schema thrift and datam...

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1919#discussion_r165809770
 
    --- Diff: format/src/main/thrift/schema.thrift ---
    @@ -192,13 +192,13 @@ struct DataMapSchema  {
         1: required string dataMapName;
         // class name
         2: required string className;
    -    // relation indentifier
    -    3: 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
    -    4: optional TableSchema childTableSchema;
         // to maintain properties like select query, query type like groupby, join
    --- End diff --
   
    change description, it stores property specified in DMPROPERTIES when creating datamap


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1919#discussion_r165809775
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/datamap/DataMapProvider.java ---
    @@ -27,6 +27,41 @@
      */
     
     public enum DataMapProvider {
    -  PREAGGREGATE,
    -  TIMESERIES;
    +  PREAGGREGATE("org.apache.carbondata.core.datamap.AggregateDataMap", "preaggregate"),
    +  TIMESERIES("org.apache.carbondata.core.datamap.TimeSeriesDataMap", "timeseries");
    +
    +  /**
    +   * Fully qualified class name of datamap
    +   */
    +  private String className;
    +
    +  /**
    +   * Short name representation of datamap
    +   */
    +  private String shortName;
    +
    +  DataMapProvider(String className, String shortName) {
    +    this.className = className;
    +    this.shortName = shortName;
    +  }
    +
    +  public String getClassName() {
    +    return className;
    +  }
    +
    +  private boolean isEqual(String dataMapClass) {
    --- End diff --
   
    also suggest change dataMapClass to  "providerName",


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1919#discussion_r165809791
 
    --- Diff: format/src/main/thrift/schema.thrift ---
    @@ -192,13 +192,13 @@ struct DataMapSchema  {
         1: required string dataMapName;
         // class name
         2: required string className;
    -    // relation indentifier
    -    3: 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
    -    4: optional TableSchema childTableSchema;
         // to maintain properties like select query, query type like groupby, join
    -    5: optional map<string, string> properties;
    +    3: optional map<string, string> properties;
    +    // relation indentifier of a table which stores data of datamaps like preaggregate/timeseries.
    --- End diff --
   
    typo, identifier


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1919#discussion_r165811152
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/datamap/DataMapProvider.java ---
    @@ -27,6 +27,41 @@
      */
     
     public enum DataMapProvider {
    -  PREAGGREGATE,
    -  TIMESERIES;
    +  PREAGGREGATE("org.apache.carbondata.core.datamap.AggregateDataMap", "preaggregate"),
    +  TIMESERIES("org.apache.carbondata.core.datamap.TimeSeriesDataMap", "timeseries");
    +
    +  /**
    +   * Fully qualified class name of datamap
    +   */
    +  private String className;
    +
    +  /**
    +   * Short name representation of datamap
    +   */
    +  private String shortName;
    +
    +  DataMapProvider(String className, String shortName) {
    +    this.className = className;
    +    this.shortName = shortName;
    +  }
    +
    +  public String getClassName() {
    +    return className;
    +  }
    +
    +  private boolean isEqual(String dataMapClass) {
    --- End diff --
   
    it can be class or provider, so no meaning of change only to provider


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1919#discussion_r165811157
 
    --- Diff: format/src/main/thrift/schema.thrift ---
    @@ -192,13 +192,13 @@ struct DataMapSchema  {
         1: required string dataMapName;
         // class name
         2: required string className;
    -    // relation indentifier
    -    3: 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
    -    4: optional TableSchema childTableSchema;
         // to maintain properties like select query, query type like groupby, join
    --- End diff --
   
    ok


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1919#discussion_r165811200
 
    --- Diff: format/src/main/thrift/schema.thrift ---
    @@ -192,13 +192,13 @@ struct DataMapSchema  {
         1: required string dataMapName;
         // class name
         2: required string className;
    -    // relation indentifier
    -    3: 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
    -    4: optional TableSchema childTableSchema;
         // to maintain properties like select query, query type like groupby, join
    -    5: optional map<string, string> properties;
    +    3: optional map<string, string> properties;
    +    // relation indentifier of a table which stores data of datamaps like preaggregate/timeseries.
    --- End diff --
   
    ok


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1919
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/2241/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user jackylk commented on the issue:

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


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

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


---