[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"
---
