[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-07-29 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r462128423



##
File path: 
integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/command/SICreationCommand.scala
##
@@ -210,6 +210,14 @@ private[sql] case class CarbonCreateSecondaryIndexCommand(
 .get
 }")
   }
+  val properties = 
carbonTable.getTableInfo.getFactTable.getTableProperties.asScala
+  val spatialProperty = properties.get(CarbonCommonConstants.SPATIAL_INDEX)
+  if (spatialProperty.isDefined) {
+if (dims.find(x => 
x.getColName.equalsIgnoreCase(spatialProperty.get.trim)).isDefined) {

Review comment:
   changed now

##
File path: 
integration/spark/src/main/scala/org/apache/spark/sql/hive/CarbonAnalysisRules.scala
##
@@ -266,16 +266,24 @@ case class CarbonPreInsertionCasts(sparkSession: 
SparkSession) extends Rule[Logi
   relation: LogicalRelation,
   child: LogicalPlan): LogicalPlan = {
 val carbonDSRelation = 
relation.relation.asInstanceOf[CarbonDatasourceHadoopRelation]
-if (carbonDSRelation.carbonRelation.output.size > CarbonCommonConstants
+val carbonTable = carbonDSRelation.carbonRelation.carbonTable
+val properties = 
carbonTable.getTableInfo.getFactTable.getTableProperties.asScala
+val spatialProperty = properties.get(CarbonCommonConstants.SPATIAL_INDEX)
+var expectedOutput = carbonDSRelation.carbonRelation.output
+// have to remove geo column to support insert with original schema

Review comment:
   same reason as above. To support inset without geo column.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-07-29 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r461343500



##
File path: 
integration/spark/src/test/scala/org/apache/carbondata/geo/GeoTest.scala
##
@@ -112,6 +238,23 @@ class GeoTest extends QueryTest with BeforeAndAfterAll 
with BeforeAndAfterEach {
   result)
   }
 
+  test("test insert into non-geo table select from geo table") {

Review comment:
   modified existing test case and added validation for the geo column.

##
File path: 
integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/command/SICreationCommand.scala
##
@@ -209,6 +209,11 @@ private[sql] case class CarbonCreateSecondaryIndexCommand(
 .get
 }")
   }
+  val isSpatialColPresent = dims.find(x => 
x.getColumnSchema.isSpatialColumn)

Review comment:
   removed isSpatialColumn from schema.

##
File path: 
integration/spark/src/main/scala/org/apache/spark/sql/hive/CarbonFileMetastore.scala
##
@@ -228,8 +230,15 @@ class CarbonFileMetastore extends CarbonMetaStore {
 
c.getClass.getName.equals("org.apache.spark.sql.catalyst.catalog.HiveTableRelation")
 ||
 c.getClass.getName.equals(
   
"org.apache.spark.sql.catalyst.catalog.UnresolvedCatalogRelation")) =>
-val catalogTable =
+var catalogTable =
   CarbonReflectionUtils.getFieldOfCatalogTable("tableMeta", 
c).asInstanceOf[CatalogTable]
+// remove spatial column from schema

Review comment:
   Here, catalogTable will have spatial column in schema which is used to 
build carbon table.  As spatial column is not supposed to be present in 
user-defined columns, removing it here. Later from tableproperties the column 
will be added in carbonTable.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-07-24 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r460033679



##
File path: 
integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertIntoCommand.scala
##
@@ -170,11 +171,18 @@ case class CarbonInsertIntoCommand(databaseNameOp: 
Option[String],
   convertedStaticPartition)
 scanResultRdd = sparkSession.sessionState.executePlan(newLogicalPlan).toRdd
 if (logicalPartitionRelation != null) {
-  if (selectedColumnSchema.length != 
logicalPartitionRelation.output.length) {
+  val properties = 
table.getTableInfo.getFactTable.getTableProperties.asScala
+  val spatialProperty = properties.get(CarbonCommonConstants.SPATIAL_INDEX)
+  var expectedOutput = logicalPartitionRelation.output
+  if (spatialProperty.isDefined && selectedColumnSchema.size + 1 == 
expectedOutput.length) {

Review comment:
   Yes, select *from table includes all columns including geoSpatial. I 
have added a testcase for that now. This change is when a user tries to insert 
with original schema. Like `sql(s"insert into $table1 select 
157542840,116285807,40084087")` . As Spatial column is not given by the 
user and is internally generated.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-07-24 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r460035762



##
File path: 
integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala
##
@@ -677,6 +677,8 @@ object CarbonParserUtil {
 val errorMsg = "range_column not support multiple columns"
 throw new MalformedCarbonCommandException(errorMsg)
   }
+  CommonUtil.validateForSpatialTypeColumn(tableProperties, rangeColumn,

Review comment:
   modified the method to validate all table properties in one place as 
suggested.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-07-24 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r460034871



##
File path: 
integration/spark/src/main/scala/org/apache/spark/sql/execution/command/mutation/CarbonProjectForUpdateCommand.scala
##
@@ -70,12 +71,20 @@ private[sql] case class CarbonProjectForUpdateCommand(
 val carbonTable = CarbonEnv.getCarbonTable(databaseNameOp, 
tableName)(sparkSession)
 setAuditTable(carbonTable)
 setAuditInfo(Map("plan" -> plan.simpleString))
+val properties = 
carbonTable.getTableInfo.getFactTable.getTableProperties.asScala
+val spatialProperty = properties.get(CarbonCommonConstants.SPATIAL_INDEX)
 columns.foreach { col =>
   val dataType = 
carbonTable.getColumnByName(col).getColumnSchema.getDataType
   if (dataType.isComplexType) {
 throw new UnsupportedOperationException("Unsupported operation on 
Complex data type")
   }
-
+  if (spatialProperty.isDefined) {
+if (col.contains(spatialProperty.get.trim)) {

Review comment:
   changed now.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-07-24 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r460034270



##
File path: 
integration/spark/src/test/scala/org/apache/carbondata/geo/GeoTest.scala
##
@@ -82,6 +81,33 @@ class GeoTest extends QueryTest with BeforeAndAfterAll with 
BeforeAndAfterEach {
   case None => assert(false)
 }
   }
+  test("test geo table drop spatial index column") {
+createTable()
+loadData()
+val exception = intercept[MalformedCarbonCommandException](sql(s"alter 
table $table1 drop columns(mygeohash)"))
+assert(exception.getMessage.contains(

Review comment:
   ok done.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-07-24 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r460033679



##
File path: 
integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertIntoCommand.scala
##
@@ -170,11 +171,18 @@ case class CarbonInsertIntoCommand(databaseNameOp: 
Option[String],
   convertedStaticPartition)
 scanResultRdd = sparkSession.sessionState.executePlan(newLogicalPlan).toRdd
 if (logicalPartitionRelation != null) {
-  if (selectedColumnSchema.length != 
logicalPartitionRelation.output.length) {
+  val properties = 
table.getTableInfo.getFactTable.getTableProperties.asScala
+  val spatialProperty = properties.get(CarbonCommonConstants.SPATIAL_INDEX)
+  var expectedOutput = logicalPartitionRelation.output
+  if (spatialProperty.isDefined && selectedColumnSchema.size + 1 == 
expectedOutput.length) {

Review comment:
   Yes, select *from table includes all columns including geoSpatial. I 
have added a testcase for that now. This change is when a user tries to insert 
with original schema. Like `sql(s"insert into $table1 select 
157542840,116285807,40084087")`





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-07-21 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r458007728



##
File path: 
integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertIntoCommand.scala
##
@@ -482,6 +490,7 @@ case class CarbonInsertIntoCommand(databaseNameOp: 
Option[String],
   null
 }
 var createOrderColumns = table.getCreateOrderColumn.asScala
+  .filterNot(_.getColumnSchema.isSpatialColumn)

Review comment:
   even now `data` and `dataFields` args of the methods don't match when 
insert with original schema.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-07-17 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r456275841



##
File path: 
integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertIntoCommand.scala
##
@@ -170,11 +171,18 @@ case class CarbonInsertIntoCommand(databaseNameOp: 
Option[String],
   convertedStaticPartition)
 scanResultRdd = sparkSession.sessionState.executePlan(newLogicalPlan).toRdd
 if (logicalPartitionRelation != null) {
-  if (selectedColumnSchema.length != 
logicalPartitionRelation.output.length) {
+  val properties = 
table.getTableInfo.getFactTable.getTableProperties.asScala

Review comment:
   In case of an insert with original schema had to remove to match with 
the input projection list.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-07-17 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r456272331



##
File path: 
integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala
##
@@ -337,7 +331,7 @@ object CarbonParserUtil {
 
 if 
(tableProperties.get(CarbonCommonConstants.COLUMN_META_CACHE).isDefined) {
   // validate the column_meta_cache option
-  val tableColumns = dims.view.filterNot(_.spatialIndex).map(x => 
x.name.get) ++
+  val tableColumns = dims.view.map(x => x.name.get) ++

Review comment:
   yes, made changes.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-07-17 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r456271810



##
File path: 
integration/spark/src/main/scala/org/apache/spark/sql/CarbonSource.scala
##
@@ -379,6 +380,17 @@ object CarbonSource {
 if (isCreatedByCarbonExtension(properties)) {
   // Table is created by SparkSession with CarbonExtension,
   // There is no TableInfo yet, so create it from CatalogTable
+  val columnSchema = updateTable.schema
+  val spatialProperty = properties.get(CarbonCommonConstants.SPATIAL_INDEX)
+  // validate for spatial index column
+  columnSchema.foreach(x => {
+if (spatialProperty.isDefined &&

Review comment:
   removed.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-06-23 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r444648223



##
File path: 
integration/spark/src/main/scala/org/apache/carbondata/spark/util/CommonUtil.scala
##
@@ -653,6 +653,21 @@ object CommonUtil {
 storeLocation
   }
 
+  def validateForSpatialTypeColumn(properties: Map[String, String],

Review comment:
   removed `validateSpatialIndexColumn()` from AlterTableUtil as validation 
to add column is no longer required, as spatial column is now part of schema.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [carbondata] ShreelekhyaG commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible

2020-06-23 Thread GitBox 


ShreelekhyaG commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r444645924



##
File path: 
integration/spark/src/test/scala/org/apache/carbondata/geo/GeoTest.scala
##
@@ -122,7 +161,8 @@ class GeoTest extends QueryTest with BeforeAndAfterAll with 
BeforeAndAfterEach {
 createTable(sourceTable)
 loadData(sourceTable)
 createTable(targetTable, "'SORT_SCOPE'='GLOBAL_SORT',")
-sql(s"insert into  $targetTable select * from $sourceTable")
+sql(s"insert into  $targetTable select timevalue, longitude," +

Review comment:
   Done





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org