[GitHub] [carbondata] ajantha-bhat commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
ajantha-bhat commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r462751220
##
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:
would have added this as comment !
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] ajantha-bhat commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
ajantha-bhat commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r460688922
##
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:
please add a test case of insert into geo table, where insert rows will
not have geo data. but select * shows geo data
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] ajantha-bhat commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
ajantha-bhat commented on a change in pull request #3774: URL: https://github.com/apache/carbondata/pull/3774#discussion_r458697336 ## 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: Please check , I think we need to remove isSpatialColumn from columnSchema. we used that mainly for making it invisible. Now as it is visible. It is just another plan column. Instead we can check if column name is in the table property or not. @VenuReddy2103 @ShreelekhyaG 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] ajantha-bhat commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
ajantha-bhat commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r458696449
##
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:
some more test case can be added with mixed case column names for other
table property validation added, example range column, bucket column etc
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] ajantha-bhat commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
ajantha-bhat commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r458695151
##
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:
same as above, we can register this column as it is visible 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] ajantha-bhat commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
ajantha-bhat commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r458694598
##
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:
I think no need to remove this column from original schema as it is a
visible column , same reason as I mentioned for `insertIntoCommand`
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] ajantha-bhat commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
ajantha-bhat commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r458690774
##
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:
why `contains` here ? it's a column name. suppose to be equalsIgnoreCase
?
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] ajantha-bhat commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
ajantha-bhat commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r458689351
##
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:
why the changes in this function ?
As user wanted to created geoSpatial column, we created an extra column,
select * from table should include all the columns.
If the target table don't want geo column, user can specify projections.
**we should not skip spatial column while doing insert into**
@VenuReddy2103 , @ShreelekhyaG
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] ajantha-bhat commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
ajantha-bhat commented on a change in pull request #3774:
URL: https://github.com/apache/carbondata/pull/3774#discussion_r458686578
##
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:
Instead of checking property by property, once all the properties are
filled, better to validate at one place ?
Also I see that for sortcolumns, column_metacache,no_inverted_index it is
not handled
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
