[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user asfgit closed the pull request at: https://github.com/apache/carbondata/pull/1919 ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
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 properties; +3: optional map properties; +// relation indentifier of a table which stores data of datamaps like preaggregate/timeseries. --- End diff -- ok ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
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 ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
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 ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
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 properties; +3: optional map properties; +// relation indentifier of a table which stores data of datamaps like preaggregate/timeseries. --- End diff -- typo, identifier ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
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 ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
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", ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user xubo245 commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165809708 --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/datamap/CarbonCreateDataMapCommand.scala --- @@ -60,7 +61,14 @@ case class CarbonCreateDataMapCommand( } else { dmProperties } - +val dataMapProvider = { + try { +DataMapProvider.getDataMapProvider(dmClassName) --- End diff -- I understand user can provide className and shortName, both belong to provider name, right? ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user ravipesala commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165809386 --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/datamap/DataMapProvider.java --- @@ -27,6 +27,40 @@ */ 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) { +return (dataMapClass != null && (dataMapClass.equals(className) || --- End diff -- ok ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user ravipesala commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165809376 --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/datamap/DataMapProvider.java --- @@ -27,6 +27,40 @@ */ 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) { +return (dataMapClass != null && (dataMapClass.equals(className) || +dataMapClass.equalsIgnoreCase(shortName))); + } + + public static DataMapProvider getDataMapProvider(String dataMapClass) { +if (TIMESERIES.isEqual(dataMapClass)) { + return TIMESERIES; +} else if (PREAGGREGATE.isEqual(dataMapClass)) { + return PREAGGREGATE; +} else { + throw new UnsupportedOperationException("Unknown data map type " + dataMapClass); --- End diff -- ok ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user ravipesala commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165809358 --- Diff: format/src/main/thrift/schema.thrift --- @@ -192,13 +192,13 @@ struct DataMapSchema { 1: required string dataMapName; // class name 2: required string className; +// to maintain properties like select query, query type like groupby, join +3: optional map properties; // relation indentifier --- End diff -- ok ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user ravipesala commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165809270 --- Diff: format/src/main/thrift/schema.thrift --- @@ -192,13 +192,13 @@ struct DataMapSchema { 1: required string dataMapName; // class name 2: required string className; +// to maintain properties like select query, query type like groupby, join +3: optional map properties; // relation indentifier -3: optional RelationIdentifier relationIdentifire; +4: optional RelationIdentifier childTableIdentifer; --- End diff -- ok ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user jackylk commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165809189 --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/datamap/DataMapProvider.java --- @@ -27,6 +27,40 @@ */ 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) { +return (dataMapClass != null && (dataMapClass.equals(className) || --- End diff -- move `(dataMapClass.equals(className) ||` to next line to make it more readible ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user jackylk commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165809145 --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/datamap/DataMapProvider.java --- @@ -27,6 +27,40 @@ */ 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) { +return (dataMapClass != null && (dataMapClass.equals(className) || +dataMapClass.equalsIgnoreCase(shortName))); + } + + public static DataMapProvider getDataMapProvider(String dataMapClass) { +if (TIMESERIES.isEqual(dataMapClass)) { + return TIMESERIES; +} else if (PREAGGREGATE.isEqual(dataMapClass)) { + return PREAGGREGATE; +} else { + throw new UnsupportedOperationException("Unknown data map type " + dataMapClass); --- End diff -- change to: `datamap provider` ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user jackylk commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165809109 --- Diff: format/src/main/thrift/schema.thrift --- @@ -192,13 +192,13 @@ struct DataMapSchema { 1: required string dataMapName; // class name 2: required string className; +// to maintain properties like select query, query type like groupby, join +3: optional map properties; // relation indentifier --- End diff -- modify the comment also, give more information to let reader know it is the relation identifier for the preaggregate table used by `preaggregate` datamap. Not used by `timeseries` datamap, right? ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user jackylk commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165809070 --- Diff: format/src/main/thrift/schema.thrift --- @@ -192,13 +192,13 @@ struct DataMapSchema { 1: required string dataMapName; // class name 2: required string className; +// to maintain properties like select query, query type like groupby, join +3: optional map properties; // relation indentifier -3: optional RelationIdentifier relationIdentifire; +4: optional RelationIdentifier childTableIdentifer; --- End diff -- typo, should be `childTableIdentifier` ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user ravipesala commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165808467 --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/datamap/CarbonCreateDataMapCommand.scala --- @@ -60,7 +61,14 @@ case class CarbonCreateDataMapCommand( } else { dmProperties } - +val dataMapProvider = { + try { +DataMapProvider.getDataMapProvider(dmClassName) --- End diff -- it could be provider name or classname, user can give any one. ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user ravipesala commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165808465 --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/datamap/DataMapProvider.java --- @@ -27,6 +27,40 @@ */ 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) { +return (dataMapClass != null && (dataMapClass.equals(className) || +dataMapClass.equalsIgnoreCase(shortName))); + } + + public static DataMapProvider getDataMapProvider(String dataMapClass) { --- End diff -- it could be provider name or classname, user can give any one. ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user xubo245 commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165807745 --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/datamap/CarbonCreateDataMapCommand.scala --- @@ -60,7 +61,14 @@ case class CarbonCreateDataMapCommand( } else { dmProperties } - +val dataMapProvider = { + try { +DataMapProvider.getDataMapProvider(dmClassName) --- End diff -- suggest change "dmClassName" to "providerName" ---
[GitHub] carbondata pull request #1919: [CARBONDATA-2123] Refactor datamap schema thr...
Github user xubo245 commented on a diff in the pull request: https://github.com/apache/carbondata/pull/1919#discussion_r165807734 --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/datamap/DataMapProvider.java --- @@ -27,6 +27,40 @@ */ 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) { +return (dataMapClass != null && (dataMapClass.equals(className) || +dataMapClass.equalsIgnoreCase(shortName))); + } + + public static DataMapProvider getDataMapProvider(String dataMapClass) { --- End diff -- suggest change "dataMapClass" to "providerName" ---