nihal0107 opened a new pull request #4000: URL: https://github.com/apache/carbondata/pull/4000 ### Why is this PR needed? Currently when we have multiple bloom indexes and we drop one index then 'show index' command is showing an empty index list. ### What changes were proposed in this PR? Checked, if no CG or FG index then set indexExist as true. ### Does this PR introduce any user interface change? - No ### Is any new testcase added? - Yes ---------------------------------------------------------------- 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: [hidden email] |
CarbonDataQA1 commented on pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#issuecomment-719433040 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4736/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA1 commented on pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#issuecomment-719435945 Build Failed with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2978/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
Indhumathi27 commented on a change in pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#discussion_r529229059 ########## File path: integration/spark/src/test/scala/org/apache/carbondata/index/bloom/BloomCoarseGrainIndexFunctionSuite.scala ########## @@ -660,6 +660,22 @@ class BloomCoarseGrainIndexFunctionSuite sql(s"SELECT * FROM $normalTable WHERE salary='1040'")) } + test("test drop index when more than one bloom index exists") { + sql(s"CREATE TABLE $bloomSampleTable " + + "(id int,name string,salary int)STORED as carbondata TBLPROPERTIES('SORT_COLUMNS'='id')") + sql(s"CREATE index index1 ON TABLE $bloomSampleTable(id) as 'bloomfilter' " + + "PROPERTIES ( 'BLOOM_SIZE'='640000', 'BLOOM_FPP'='0.00001', 'BLOOM_COMPRESS'='true')") + sql(s"CREATE index index2 ON TABLE $bloomSampleTable (name) as 'bloomfilter' " + + "PROPERTIES ('BLOOM_SIZE'='640000', 'BLOOM_FPP'='0.00001', 'BLOOM_COMPRESS'='true')") + sql(s"insert into $bloomSampleTable values(1,'nihal',20)") + sql(s"SHOW INDEXES ON TABLE $bloomSampleTable").collect() + checkExistence(sql(s"SHOW INDEXES ON TABLE $bloomSampleTable"), true, "index1") Review comment: ```suggestion checkExistence(sql(s"SHOW INDEXES ON TABLE $bloomSampleTable"), true, "index1", "index2" ) ``` Remove next line ########## File path: integration/spark/src/test/scala/org/apache/carbondata/index/bloom/BloomCoarseGrainIndexFunctionSuite.scala ########## @@ -660,6 +660,22 @@ class BloomCoarseGrainIndexFunctionSuite sql(s"SELECT * FROM $normalTable WHERE salary='1040'")) } + test("test drop index when more than one bloom index exists") { + sql(s"CREATE TABLE $bloomSampleTable " + + "(id int,name string,salary int)STORED as carbondata TBLPROPERTIES('SORT_COLUMNS'='id')") + sql(s"CREATE index index1 ON TABLE $bloomSampleTable(id) as 'bloomfilter' " + + "PROPERTIES ( 'BLOOM_SIZE'='640000', 'BLOOM_FPP'='0.00001', 'BLOOM_COMPRESS'='true')") + sql(s"CREATE index index2 ON TABLE $bloomSampleTable (name) as 'bloomfilter' " + + "PROPERTIES ('BLOOM_SIZE'='640000', 'BLOOM_FPP'='0.00001', 'BLOOM_COMPRESS'='true')") + sql(s"insert into $bloomSampleTable values(1,'nihal',20)") + sql(s"SHOW INDEXES ON TABLE $bloomSampleTable").collect() + checkExistence(sql(s"SHOW INDEXES ON TABLE $bloomSampleTable"), true, "index1") + checkExistence(sql(s"SHOW INDEXES ON TABLE $bloomSampleTable"), true, "index2") + sql(s"drop index index1 on $bloomSampleTable") + sql(s"show indexes on table $bloomSampleTable").show() Review comment: remove this line ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/index/DropIndexCommand.scala ########## @@ -184,10 +184,10 @@ private[sql] case class DropIndexCommand( parentCarbonTable = getRefreshedParentTable(sparkSession, dbName) val indexMetadata = parentCarbonTable.getIndexMetadata if (null != indexMetadata && null != indexMetadata.getIndexesMap) { - val hasCgFgIndexes = - !(indexMetadata.getIndexesMap.size() == 1 && - indexMetadata.getIndexesMap.containsKey(IndexType.SI.getIndexProviderName)) - if (hasCgFgIndexes) { + val hasCgFgIndexes = indexMetadata.getIndexesMap.size() != 0 && Review comment: Please add a comment, on which case, we need to set 'indexExists' to false ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
nihal0107 commented on a change in pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#discussion_r529518625 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/index/DropIndexCommand.scala ########## @@ -184,10 +184,10 @@ private[sql] case class DropIndexCommand( parentCarbonTable = getRefreshedParentTable(sparkSession, dbName) val indexMetadata = parentCarbonTable.getIndexMetadata if (null != indexMetadata && null != indexMetadata.getIndexesMap) { - val hasCgFgIndexes = - !(indexMetadata.getIndexesMap.size() == 1 && - indexMetadata.getIndexesMap.containsKey(IndexType.SI.getIndexProviderName)) - if (hasCgFgIndexes) { + val hasCgFgIndexes = indexMetadata.getIndexesMap.size() != 0 && Review comment: added. ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
nihal0107 commented on a change in pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#discussion_r529518888 ########## File path: integration/spark/src/test/scala/org/apache/carbondata/index/bloom/BloomCoarseGrainIndexFunctionSuite.scala ########## @@ -660,6 +660,22 @@ class BloomCoarseGrainIndexFunctionSuite sql(s"SELECT * FROM $normalTable WHERE salary='1040'")) } + test("test drop index when more than one bloom index exists") { + sql(s"CREATE TABLE $bloomSampleTable " + + "(id int,name string,salary int)STORED as carbondata TBLPROPERTIES('SORT_COLUMNS'='id')") + sql(s"CREATE index index1 ON TABLE $bloomSampleTable(id) as 'bloomfilter' " + + "PROPERTIES ( 'BLOOM_SIZE'='640000', 'BLOOM_FPP'='0.00001', 'BLOOM_COMPRESS'='true')") + sql(s"CREATE index index2 ON TABLE $bloomSampleTable (name) as 'bloomfilter' " + + "PROPERTIES ('BLOOM_SIZE'='640000', 'BLOOM_FPP'='0.00001', 'BLOOM_COMPRESS'='true')") + sql(s"insert into $bloomSampleTable values(1,'nihal',20)") + sql(s"SHOW INDEXES ON TABLE $bloomSampleTable").collect() + checkExistence(sql(s"SHOW INDEXES ON TABLE $bloomSampleTable"), true, "index1") Review comment: done ########## File path: integration/spark/src/test/scala/org/apache/carbondata/index/bloom/BloomCoarseGrainIndexFunctionSuite.scala ########## @@ -660,6 +660,22 @@ class BloomCoarseGrainIndexFunctionSuite sql(s"SELECT * FROM $normalTable WHERE salary='1040'")) } + test("test drop index when more than one bloom index exists") { + sql(s"CREATE TABLE $bloomSampleTable " + + "(id int,name string,salary int)STORED as carbondata TBLPROPERTIES('SORT_COLUMNS'='id')") + sql(s"CREATE index index1 ON TABLE $bloomSampleTable(id) as 'bloomfilter' " + + "PROPERTIES ( 'BLOOM_SIZE'='640000', 'BLOOM_FPP'='0.00001', 'BLOOM_COMPRESS'='true')") + sql(s"CREATE index index2 ON TABLE $bloomSampleTable (name) as 'bloomfilter' " + + "PROPERTIES ('BLOOM_SIZE'='640000', 'BLOOM_FPP'='0.00001', 'BLOOM_COMPRESS'='true')") + sql(s"insert into $bloomSampleTable values(1,'nihal',20)") + sql(s"SHOW INDEXES ON TABLE $bloomSampleTable").collect() + checkExistence(sql(s"SHOW INDEXES ON TABLE $bloomSampleTable"), true, "index1") + checkExistence(sql(s"SHOW INDEXES ON TABLE $bloomSampleTable"), true, "index2") + sql(s"drop index index1 on $bloomSampleTable") + sql(s"show indexes on table $bloomSampleTable").show() 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#issuecomment-733012325 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4889/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#issuecomment-733019583 Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3135/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
VenuReddy2103 commented on a change in pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#discussion_r531399137 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/index/DropIndexCommand.scala ########## @@ -184,10 +184,12 @@ private[sql] case class DropIndexCommand( parentCarbonTable = getRefreshedParentTable(sparkSession, dbName) val indexMetadata = parentCarbonTable.getIndexMetadata if (null != indexMetadata && null != indexMetadata.getIndexesMap) { - val hasCgFgIndexes = - !(indexMetadata.getIndexesMap.size() == 1 && - indexMetadata.getIndexesMap.containsKey(IndexType.SI.getIndexProviderName)) - if (hasCgFgIndexes) { + // check if any CG or FG index exists. If not exists, + // then set indexExists as false to return empty index list for next query. + val hasCgFgIndexes = indexMetadata.getIndexesMap.size() != 0 && Review comment: `indexMetadata.getIndexesMap.size() != 0` would always be true at this point. `indexMetadata` will be null if empty. ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
VenuReddy2103 commented on a change in pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#discussion_r531399137 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/index/DropIndexCommand.scala ########## @@ -184,10 +184,12 @@ private[sql] case class DropIndexCommand( parentCarbonTable = getRefreshedParentTable(sparkSession, dbName) val indexMetadata = parentCarbonTable.getIndexMetadata if (null != indexMetadata && null != indexMetadata.getIndexesMap) { - val hasCgFgIndexes = - !(indexMetadata.getIndexesMap.size() == 1 && - indexMetadata.getIndexesMap.containsKey(IndexType.SI.getIndexProviderName)) - if (hasCgFgIndexes) { + // check if any CG or FG index exists. If not exists, + // then set indexExists as false to return empty index list for next query. + val hasCgFgIndexes = indexMetadata.getIndexesMap.size() != 0 && Review comment: `indexMetadata.getIndexesMap.size() != 0` would always be true at this point. `indexMetadata` will be null if empty. It is redundant check. ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
VenuReddy2103 commented on a change in pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#discussion_r531409321 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/index/DropIndexCommand.scala ########## @@ -184,10 +184,12 @@ private[sql] case class DropIndexCommand( parentCarbonTable = getRefreshedParentTable(sparkSession, dbName) val indexMetadata = parentCarbonTable.getIndexMetadata if (null != indexMetadata && null != indexMetadata.getIndexesMap) { - val hasCgFgIndexes = - !(indexMetadata.getIndexesMap.size() == 1 && - indexMetadata.getIndexesMap.containsKey(IndexType.SI.getIndexProviderName)) - if (hasCgFgIndexes) { + // check if any CG or FG index exists. If not exists, Review comment: For example, create 2 bloom indexes, Drop both of them. In last index drop, `indexMetadata` will be null at this point(line 186). We do not seem to set `indexExists` property to `false` in that case. ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
Indhumathi27 commented on a change in pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#discussion_r531409958 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/index/DropIndexCommand.scala ########## @@ -184,10 +184,12 @@ private[sql] case class DropIndexCommand( parentCarbonTable = getRefreshedParentTable(sparkSession, dbName) val indexMetadata = parentCarbonTable.getIndexMetadata if (null != indexMetadata && null != indexMetadata.getIndexesMap) { - val hasCgFgIndexes = - !(indexMetadata.getIndexesMap.size() == 1 && - indexMetadata.getIndexesMap.containsKey(IndexType.SI.getIndexProviderName)) - if (hasCgFgIndexes) { + // check if any CG or FG index exists. If not exists, + // then set indexExists as false to return empty index list for next query. + val hasCgFgIndexes = indexMetadata.getIndexesMap.size() != 0 && Review comment: indexmetadata will also have SI Indexes. indexExists property is only for CG or FG indexes ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
VenuReddy2103 commented on a change in pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#discussion_r531416935 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/index/DropIndexCommand.scala ########## @@ -184,10 +184,12 @@ private[sql] case class DropIndexCommand( parentCarbonTable = getRefreshedParentTable(sparkSession, dbName) val indexMetadata = parentCarbonTable.getIndexMetadata if (null != indexMetadata && null != indexMetadata.getIndexesMap) { - val hasCgFgIndexes = - !(indexMetadata.getIndexesMap.size() == 1 && - indexMetadata.getIndexesMap.containsKey(IndexType.SI.getIndexProviderName)) - if (hasCgFgIndexes) { + // check if any CG or FG index exists. If not exists, + // then set indexExists as false to return empty index list for next query. + val hasCgFgIndexes = indexMetadata.getIndexesMap.size() != 0 && Review comment: Got it. ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
VenuReddy2103 commented on a change in pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#discussion_r531416935 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/index/DropIndexCommand.scala ########## @@ -184,10 +184,12 @@ private[sql] case class DropIndexCommand( parentCarbonTable = getRefreshedParentTable(sparkSession, dbName) val indexMetadata = parentCarbonTable.getIndexMetadata if (null != indexMetadata && null != indexMetadata.getIndexesMap) { - val hasCgFgIndexes = - !(indexMetadata.getIndexesMap.size() == 1 && - indexMetadata.getIndexesMap.containsKey(IndexType.SI.getIndexProviderName)) - if (hasCgFgIndexes) { + // check if any CG or FG index exists. If not exists, + // then set indexExists as false to return empty index list for next query. + val hasCgFgIndexes = indexMetadata.getIndexesMap.size() != 0 && Review comment: I understand that `indexMetadata` will have SI indexes as well. But, What i meant was indexMetadata.getIndexesMap.size() != 0 always evaluates to true at line 189. ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
nihal0107 commented on a change in pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#discussion_r531434575 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/index/DropIndexCommand.scala ########## @@ -184,10 +184,12 @@ private[sql] case class DropIndexCommand( parentCarbonTable = getRefreshedParentTable(sparkSession, dbName) val indexMetadata = parentCarbonTable.getIndexMetadata if (null != indexMetadata && null != indexMetadata.getIndexesMap) { - val hasCgFgIndexes = - !(indexMetadata.getIndexesMap.size() == 1 && - indexMetadata.getIndexesMap.containsKey(IndexType.SI.getIndexProviderName)) - if (hasCgFgIndexes) { + // check if any CG or FG index exists. If not exists, Review comment: ok, handle the scenario when no cg or fg index exists then set `indexExists` property as false. Earlier this case was not handled when we drop all indexes then we didn't set the `indexExists` property as false. ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#issuecomment-734743170 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4950/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#issuecomment-734746457 Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3195/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
nihal0107 commented on a change in pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#discussion_r531507427 ########## File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/index/DropIndexCommand.scala ########## @@ -184,10 +184,12 @@ private[sql] case class DropIndexCommand( parentCarbonTable = getRefreshedParentTable(sparkSession, dbName) val indexMetadata = parentCarbonTable.getIndexMetadata if (null != indexMetadata && null != indexMetadata.getIndexesMap) { - val hasCgFgIndexes = - !(indexMetadata.getIndexesMap.size() == 1 && - indexMetadata.getIndexesMap.containsKey(IndexType.SI.getIndexProviderName)) - if (hasCgFgIndexes) { + // check if any CG or FG index exists. If not exists, + // then set indexExists as false to return empty index list for next query. + val hasCgFgIndexes = indexMetadata.getIndexesMap.size() != 0 && Review comment: ok, removed the check because indexes map can have max 3 elements and it won't throw any exception for empty map. ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#issuecomment-734801021 Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4955/ ---------------------------------------------------------------- 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: [hidden email] |
In reply to this post by GitBox
CarbonDataQA2 commented on pull request #4000: URL: https://github.com/apache/carbondata/pull/4000#issuecomment-734801467 Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/3200/ ---------------------------------------------------------------- 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: [hidden email] |
Free forum by Nabble | Edit this page |