[GitHub] [carbondata] VenuReddy2103 commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
VenuReddy2103 commented on a change in pull request #3774: URL: https://github.com/apache/carbondata/pull/3774#discussion_r461692563 ## 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: you would want to check for index column names. 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] VenuReddy2103 commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
VenuReddy2103 commented on a change in pull request #3774: URL: https://github.com/apache/carbondata/pull/3774#discussion_r437700014 ## 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: Please refer above comment. Also you would want to revisit `convertToNoDictionaryToBytes()` and `convertToNoDictionaryToBytesWithoutReArrange()` method in `InputProcessorStepWithNoConverterImpl` class. Without this PR, length of `data` and `dataFields` args of those method were not matching as we not including spatial column. so we had some change in them. With your PR it can be simplified. Please refer PR #3760. 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] VenuReddy2103 commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
VenuReddy2103 commented on a change in pull request #3774: URL: https://github.com/apache/carbondata/pull/3774#discussion_r437599439 ## File path: integration/spark/src/test/scala/org/apache/carbondata/geo/GeoTest.scala ## @@ -82,6 +81,45 @@ 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( + s"Columns present in ${CarbonCommonConstants.SPATIAL_INDEX} table property cannot be altered.")) + } + + test("test geo table filter by geo spatial index column") { +createTable() +loadData() +checkAnswer(sql(s"select *from $table1 where mygeohash = '2196036'"), + Seq(Row(2196036,157542840L,116337069,39951887))) + } + + test("test geo table create index on geohandler column") { +createTable() +loadData() +val exception = intercept[MalformedIndexCommandException](sql( + s""" + | CREATE INDEX bloom_index ON TABLE $table1 (mygeohash) + | AS 'bloomfilter' + | PROPERTIES('BLOOM_SIZE'='64', 'BLOOM_FPP'='0.1') + """.stripMargin)) + assert(exception.getMessage.contains( + s"Spatial Index column is not supported, column 'mygeohash' is spatial column")) + } + + test("test geo table custom properties on geohandler column") { +try { + createTable(table1,"'COLUMN_META_CACHE' = 'mygeohash',") Review comment: Test framework supports interction of exceptions. Instead of this try catch and assert, please use intercept like above testcases. 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] VenuReddy2103 commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
VenuReddy2103 commented on a change in pull request #3774: URL: https://github.com/apache/carbondata/pull/3774#discussion_r437595830 ## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CommonLoadUtils.scala ## @@ -568,8 +567,7 @@ object CommonLoadUtils { .getColumnSchemaList.size())) .asInstanceOf[Array[ColumnSchema]] } else { - colSchema = colSchema.filterNot(x => x.isInvisible || x.isComplexColumn || - x.getSchemaOrdinal == -1) + colSchema = colSchema.filterNot(x => x.isInvisible || x.isComplexColumn) Review comment: why remove getSchemaOrdinal check? we do not judge spatial column by schema ordinal as -1. And x.getSchemaOrdinal was not introduced with spatial support PR. Same applies to if case above. 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] VenuReddy2103 commented on a change in pull request #3774: [CARBONDATA-3833] Make geoID visible
VenuReddy2103 commented on a change in pull request #3774: URL: https://github.com/apache/carbondata/pull/3774#discussion_r437199601 ## 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: why select * is changed to individual columns ? Probably because you get geospatial column as well when you give select *. But, we need to handle this insert into select *. ## 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: `columnPropert` string is just to use that in exception message. we alreday have a similar method `validateSpatialIndexColumn()` for it. Suggest to reuse. ## 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: `columnPropert` string is just to use that in exception message. we alreday have a similar method `validateSpatialIndexColumn()` for it. Suggest to modify existing and reuse instead of complete new method for the same validation. ## 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: instead of `spatialProperty.isDefined`check inside the loop it should be outside. We can avoid unnecessary loop traversal if the property itself is not defined. ## 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 Review comment: why moved this exception case alone here from `processSpatialIndexProperty` ? Probably we have missed something here. I think, we have many other invalid property exceptions in `processSpatialIndexProperty` and even for other properties in `prepareTableModel()`. ## File path: integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala ## @@ -213,8 +207,8 @@ object CarbonParserUtil { // Process spatial index property val indexFields = processSpatialIndexProperty(tableProperties, fields) -val allFields = fields ++ indexFields +val allFields = (fields ++ indexFields).distinct Review comment: why distinct ? have already checked that indexFields are not in actual fields. right ? If required please add the comments here. ## File path: integration/spark/src/main/scala/org/apache/spark/sql/catalyst/CarbonParserUtil.scala ## @@ -213,8 +207,8 @@ object CarbonParserUtil { // Process spatial index property val indexFields = processSpatialIndexProperty(tableProperties, fields) -val allFields = fields ++ indexFields +val allFields = (fields ++ indexFields).distinct Review comment: why distinct ? have already checked that indexFields are not in actual fields. right ? If required please add the comments here. Same applies for another instance below. ## 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: if we had not removed this filter, then you wouldn't require modification in `validateColumnMetaCacheFields` or `validateColumnMetaCacheOption` i guess. ## File path: integration/spark/src/main/scala/org/apache/sp