[GitHub] [carbondata] ajantha-bhat opened a new pull request #3787: [WIP] support sort_scope for index creation

[GitBox]

ajantha-bhat commented on a change in pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#discussion_r486266977



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/strategy/CarbonLateDecodeStrategy.scala
##########
@@ -89,7 +104,7 @@ private[sql] class CarbonLateDecodeStrategy extends SparkStrategy {
         try {
           pruneFilterProject(
             l,
-            projects.filterNot(_.name.equalsIgnoreCase(CarbonCommonConstants.POSITION_ID)),

Review comment:
       Because these position id should not be removed always, it has to be removed only in certain conditions (example, when `isPositionIDRequested` property is not set)
   
   Inside this `getRequestedColumns` will take care of removing it based on conditions.




----------------------------------------------------------------
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]

[GitBox]

akashrn5 commented on a change in pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#discussion_r486269619



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -86,6 +86,64 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
       .contains("Alter table drop column operation failed:"))
   }
 
+  test("test create secondary index global sort after insert") {
+    sql("drop table if exists table1")
+    sql("create table table1 (name string, id string, country string) stored as carbondata")
+    sql("insert into table1 select 'xx', '2', 'china' union all select 'xx', '1', 'india'")
+    sql("create index table1_index on table table1(id, country) as 'carbondata' properties" +
+        "('sort_scope'='global_sort', 'Global_sort_partitions'='3')")
+    checkAnswerWithoutSort(sql("select id, country from table1_index"),
+      Seq(Row("1", "india"), Row("2", "china")))
+    // check for valid sort_scope
+    checkExistence(sql("describe formatted table1_index"), true, "Sort Scope global_sort")
+    // check the invalid sort scope
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='tim_sort', 'Global_sort_partitions'='3')"))
+      .getMessage
+      .contains("Invalid SORT_SCOPE tim_sort"))
+    // check for invalid global_sort_partitions
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='global_sort', 'Global_sort_partitions'='-1')"))
+      .getMessage
+      .contains("Table property global_sort_partitions : -1 is invalid"))
+    sql("drop index table1_index on table1")

Review comment:
       a) not talking about overhead, why to call the command when that will be handled by drop table, why to take effort to call another command, please remove it and same for other test case.
   b) even though its not an example file, we  should always give proper and meaningful names. Just because user uses carbon and see code, we cant give non meaningful names right...!!!




----------------------------------------------------------------
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]

[GitBox]

CarbonDataQA1 commented on pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#issuecomment-690216620


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/4036/
   


----------------------------------------------------------------
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]

[GitBox]

ajantha-bhat commented on a change in pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#discussion_r486276366



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -86,6 +86,64 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
       .contains("Alter table drop column operation failed:"))
   }
 
+  test("test create secondary index global sort after insert") {
+    sql("drop table if exists table1")
+    sql("create table table1 (name string, id string, country string) stored as carbondata")
+    sql("insert into table1 select 'xx', '2', 'china' union all select 'xx', '1', 'india'")
+    sql("create index table1_index on table table1(id, country) as 'carbondata' properties" +
+        "('sort_scope'='global_sort', 'Global_sort_partitions'='3')")
+    checkAnswerWithoutSort(sql("select id, country from table1_index"),
+      Seq(Row("1", "india"), Row("2", "china")))
+    // check for valid sort_scope
+    checkExistence(sql("describe formatted table1_index"), true, "Sort Scope global_sort")
+    // check the invalid sort scope
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='tim_sort', 'Global_sort_partitions'='3')"))
+      .getMessage
+      .contains("Invalid SORT_SCOPE tim_sort"))
+    // check for invalid global_sort_partitions
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='global_sort', 'Global_sort_partitions'='-1')"))
+      .getMessage
+      .contains("Table property global_sort_partitions : -1 is invalid"))
+    sql("drop index table1_index on table1")

Review comment:
       >  we cant give non meaningful names right...!!!
   table1 is a meaningful name to represent it as a table, it is like john wick calling his dog as a dog. On a lighter note, Stop focusing on unimportant things. As an experienced developer, I do know when a code is not readable.




----------------------------------------------------------------
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]

[GitBox]

ajantha-bhat commented on a change in pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#discussion_r486276366



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -86,6 +86,64 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
       .contains("Alter table drop column operation failed:"))
   }
 
+  test("test create secondary index global sort after insert") {
+    sql("drop table if exists table1")
+    sql("create table table1 (name string, id string, country string) stored as carbondata")
+    sql("insert into table1 select 'xx', '2', 'china' union all select 'xx', '1', 'india'")
+    sql("create index table1_index on table table1(id, country) as 'carbondata' properties" +
+        "('sort_scope'='global_sort', 'Global_sort_partitions'='3')")
+    checkAnswerWithoutSort(sql("select id, country from table1_index"),
+      Seq(Row("1", "india"), Row("2", "china")))
+    // check for valid sort_scope
+    checkExistence(sql("describe formatted table1_index"), true, "Sort Scope global_sort")
+    // check the invalid sort scope
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='tim_sort', 'Global_sort_partitions'='3')"))
+      .getMessage
+      .contains("Invalid SORT_SCOPE tim_sort"))
+    // check for invalid global_sort_partitions
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='global_sort', 'Global_sort_partitions'='-1')"))
+      .getMessage
+      .contains("Table property global_sort_partitions : -1 is invalid"))
+    sql("drop index table1_index on table1")

Review comment:
       >  we cant give non meaningful names right...!!!
   
   table1 is a meaningful name to represent it as a table, it is like john wick calling his dog as a dog. On a lighter note, Stop focusing on unimportant things (table1 is used in other 100 places also). As an experienced developer, I do know when a code is not readable.




----------------------------------------------------------------
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]

[GitBox]

CarbonDataQA1 commented on pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#issuecomment-690218157


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.5/2297/
   


----------------------------------------------------------------
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]

[GitBox]

akashrn5 commented on a change in pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#discussion_r486279565



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -86,6 +86,64 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
       .contains("Alter table drop column operation failed:"))
   }
 
+  test("test create secondary index global sort after insert") {
+    sql("drop table if exists table1")
+    sql("create table table1 (name string, id string, country string) stored as carbondata")
+    sql("insert into table1 select 'xx', '2', 'china' union all select 'xx', '1', 'india'")
+    sql("create index table1_index on table table1(id, country) as 'carbondata' properties" +
+        "('sort_scope'='global_sort', 'Global_sort_partitions'='3')")
+    checkAnswerWithoutSort(sql("select id, country from table1_index"),
+      Seq(Row("1", "india"), Row("2", "china")))
+    // check for valid sort_scope
+    checkExistence(sql("describe formatted table1_index"), true, "Sort Scope global_sort")
+    // check the invalid sort scope
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='tim_sort', 'Global_sort_partitions'='3')"))
+      .getMessage
+      .contains("Invalid SORT_SCOPE tim_sort"))
+    // check for invalid global_sort_partitions
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='global_sort', 'Global_sort_partitions'='-1')"))
+      .getMessage
+      .contains("Table property global_sort_partitions : -1 is invalid"))
+    sql("drop index table1_index on table1")

Review comment:
       its not about the experience, but i always prefer the code to be very clean and meaningful and reader or developer should be happy reading about it and clean and meaningful names are very important aspect in any code...!




----------------------------------------------------------------
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]

[GitBox]

akashrn5 commented on a change in pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#discussion_r486279565



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -86,6 +86,64 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
       .contains("Alter table drop column operation failed:"))
   }
 
+  test("test create secondary index global sort after insert") {
+    sql("drop table if exists table1")
+    sql("create table table1 (name string, id string, country string) stored as carbondata")
+    sql("insert into table1 select 'xx', '2', 'china' union all select 'xx', '1', 'india'")
+    sql("create index table1_index on table table1(id, country) as 'carbondata' properties" +
+        "('sort_scope'='global_sort', 'Global_sort_partitions'='3')")
+    checkAnswerWithoutSort(sql("select id, country from table1_index"),
+      Seq(Row("1", "india"), Row("2", "china")))
+    // check for valid sort_scope
+    checkExistence(sql("describe formatted table1_index"), true, "Sort Scope global_sort")
+    // check the invalid sort scope
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='tim_sort', 'Global_sort_partitions'='3')"))
+      .getMessage
+      .contains("Invalid SORT_SCOPE tim_sort"))
+    // check for invalid global_sort_partitions
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='global_sort', 'Global_sort_partitions'='-1')"))
+      .getMessage
+      .contains("Table property global_sort_partitions : -1 is invalid"))
+    sql("drop index table1_index on table1")

Review comment:
       its not about the experience, but i always prefer the code to be very clean and meaningful and reader or developer should be happy reading it. Clean and meaningful names are very important aspect in any code...!




----------------------------------------------------------------
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]

[GitBox]

ajantha-bhat commented on a change in pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#discussion_r486289292



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -86,6 +86,64 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
       .contains("Alter table drop column operation failed:"))
   }
 
+  test("test create secondary index global sort after insert") {
+    sql("drop table if exists table1")
+    sql("create table table1 (name string, id string, country string) stored as carbondata")
+    sql("insert into table1 select 'xx', '2', 'china' union all select 'xx', '1', 'india'")
+    sql("create index table1_index on table table1(id, country) as 'carbondata' properties" +
+        "('sort_scope'='global_sort', 'Global_sort_partitions'='3')")
+    checkAnswerWithoutSort(sql("select id, country from table1_index"),
+      Seq(Row("1", "india"), Row("2", "china")))
+    // check for valid sort_scope
+    checkExistence(sql("describe formatted table1_index"), true, "Sort Scope global_sort")
+    // check the invalid sort scope
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='tim_sort', 'Global_sort_partitions'='3')"))
+      .getMessage
+      .contains("Invalid SORT_SCOPE tim_sort"))
+    // check for invalid global_sort_partitions
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='global_sort', 'Global_sort_partitions'='-1')"))
+      .getMessage
+      .contains("Table property global_sort_partitions : -1 is invalid"))
+    sql("drop index table1_index on table1")

Review comment:
       I can see that from your PR #3608 , you have used t1 as table name and i1 as index name in DropTableTest. is that clean and meaningful name ? I don't want to argue further.
   Table1 is still a table name I have not named it as car or bike.




----------------------------------------------------------------
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]

[GitBox]

akashrn5 commented on a change in pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#discussion_r486228364



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -86,6 +86,64 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
       .contains("Alter table drop column operation failed:"))
   }
 
+  test("test create secondary index global sort after insert") {
+    sql("drop table if exists table1")
+    sql("create table table1 (name string, id string, country string) stored as carbondata")
+    sql("insert into table1 select 'xx', '2', 'china' union all select 'xx', '1', 'india'")
+    sql("create index table1_index on table table1(id, country) as 'carbondata' properties" +
+        "('sort_scope'='global_sort', 'Global_sort_partitions'='3')")
+    checkAnswerWithoutSort(sql("select id, country from table1_index"),
+      Seq(Row("1", "india"), Row("2", "china")))
+    // check for valid sort_scope
+    checkExistence(sql("describe formatted table1_index"), true, "Sort Scope global_sort")
+    // check the invalid sort scope
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='tim_sort', 'Global_sort_partitions'='3')"))
+      .getMessage
+      .contains("Invalid SORT_SCOPE tim_sort"))
+    // check for invalid global_sort_partitions
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='global_sort', 'Global_sort_partitions'='-1')"))
+      .getMessage
+      .contains("Table property global_sort_partitions : -1 is invalid"))
+    sql("drop index table1_index on table1")

Review comment:
       can just do drop table, it will drop index too, no need to separately run drop index and suggest to give a better tableName and index name and please check other test for same input.

##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -86,6 +86,64 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
       .contains("Alter table drop column operation failed:"))
   }
 
+  test("test create secondary index global sort after insert") {
+    sql("drop table if exists table1")
+    sql("create table table1 (name string, id string, country string) stored as carbondata")
+    sql("insert into table1 select 'xx', '2', 'china' union all select 'xx', '1', 'india'")
+    sql("create index table1_index on table table1(id, country) as 'carbondata' properties" +
+        "('sort_scope'='global_sort', 'Global_sort_partitions'='3')")
+    checkAnswerWithoutSort(sql("select id, country from table1_index"),
+      Seq(Row("1", "india"), Row("2", "china")))
+    // check for valid sort_scope
+    checkExistence(sql("describe formatted table1_index"), true, "Sort Scope global_sort")
+    // check the invalid sort scope
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='tim_sort', 'Global_sort_partitions'='3')"))
+      .getMessage
+      .contains("Invalid SORT_SCOPE tim_sort"))
+    // check for invalid global_sort_partitions
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='global_sort', 'Global_sort_partitions'='-1')"))
+      .getMessage
+      .contains("Table property global_sort_partitions : -1 is invalid"))
+    sql("drop index table1_index on table1")

Review comment:
       a) not talking about overhead, why to call the command when that will be handled by drop table, why to take effort to call another command, please remove it and same for other test case.
   b) even though its not an example file, we  should always give proper and meaningful names. Just because user uses carbon and see code, we cant give non meaningful names right...!!!

##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -86,6 +86,64 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
       .contains("Alter table drop column operation failed:"))
   }
 
+  test("test create secondary index global sort after insert") {
+    sql("drop table if exists table1")
+    sql("create table table1 (name string, id string, country string) stored as carbondata")
+    sql("insert into table1 select 'xx', '2', 'china' union all select 'xx', '1', 'india'")
+    sql("create index table1_index on table table1(id, country) as 'carbondata' properties" +
+        "('sort_scope'='global_sort', 'Global_sort_partitions'='3')")
+    checkAnswerWithoutSort(sql("select id, country from table1_index"),
+      Seq(Row("1", "india"), Row("2", "china")))
+    // check for valid sort_scope
+    checkExistence(sql("describe formatted table1_index"), true, "Sort Scope global_sort")
+    // check the invalid sort scope
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='tim_sort', 'Global_sort_partitions'='3')"))
+      .getMessage
+      .contains("Invalid SORT_SCOPE tim_sort"))
+    // check for invalid global_sort_partitions
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='global_sort', 'Global_sort_partitions'='-1')"))
+      .getMessage
+      .contains("Table property global_sort_partitions : -1 is invalid"))
+    sql("drop index table1_index on table1")

Review comment:
       its not about the experience, but i always prefer the code to be very clean and meaningful and reader or developer should be happy reading it. Clean and meaningful names are very important aspect in any code...!




----------------------------------------------------------------
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]

[GitBox]

ajantha-bhat commented on a change in pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#discussion_r486264018



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -86,6 +86,64 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
       .contains("Alter table drop column operation failed:"))
   }
 
+  test("test create secondary index global sort after insert") {
+    sql("drop table if exists table1")
+    sql("create table table1 (name string, id string, country string) stored as carbondata")
+    sql("insert into table1 select 'xx', '2', 'china' union all select 'xx', '1', 'india'")
+    sql("create index table1_index on table table1(id, country) as 'carbondata' properties" +
+        "('sort_scope'='global_sort', 'Global_sort_partitions'='3')")
+    checkAnswerWithoutSort(sql("select id, country from table1_index"),
+      Seq(Row("1", "india"), Row("2", "china")))
+    // check for valid sort_scope
+    checkExistence(sql("describe formatted table1_index"), true, "Sort Scope global_sort")
+    // check the invalid sort scope
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='tim_sort', 'Global_sort_partitions'='3')"))
+      .getMessage
+      .contains("Invalid SORT_SCOPE tim_sort"))
+    // check for invalid global_sort_partitions
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='global_sort', 'Global_sort_partitions'='-1')"))
+      .getMessage
+      .contains("Table property global_sort_partitions : -1 is invalid"))
+    sql("drop index table1_index on table1")

Review comment:
       a) I know, but calling drop index will not add extra overhead.
   b) For test cases these names are enough ! This is not an example file.

##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -86,6 +86,64 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
       .contains("Alter table drop column operation failed:"))
   }
 
+  test("test create secondary index global sort after insert") {
+    sql("drop table if exists table1")
+    sql("create table table1 (name string, id string, country string) stored as carbondata")
+    sql("insert into table1 select 'xx', '2', 'china' union all select 'xx', '1', 'india'")
+    sql("create index table1_index on table table1(id, country) as 'carbondata' properties" +
+        "('sort_scope'='global_sort', 'Global_sort_partitions'='3')")
+    checkAnswerWithoutSort(sql("select id, country from table1_index"),
+      Seq(Row("1", "india"), Row("2", "china")))
+    // check for valid sort_scope
+    checkExistence(sql("describe formatted table1_index"), true, "Sort Scope global_sort")
+    // check the invalid sort scope
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='tim_sort', 'Global_sort_partitions'='3')"))
+      .getMessage
+      .contains("Invalid SORT_SCOPE tim_sort"))
+    // check for invalid global_sort_partitions
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='global_sort', 'Global_sort_partitions'='-1')"))
+      .getMessage
+      .contains("Table property global_sort_partitions : -1 is invalid"))
+    sql("drop index table1_index on table1")

Review comment:
       >  we cant give non meaningful names right...!!!
   
   table1 is a meaningful name to represent it as a table, it is like john wick calling his dog as a dog. On a lighter note, Stop focusing on unimportant things (table1 is used in other 100 places also). As an experienced developer, I do know when a code is not readable.

##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -86,6 +86,64 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
       .contains("Alter table drop column operation failed:"))
   }
 
+  test("test create secondary index global sort after insert") {
+    sql("drop table if exists table1")
+    sql("create table table1 (name string, id string, country string) stored as carbondata")
+    sql("insert into table1 select 'xx', '2', 'china' union all select 'xx', '1', 'india'")
+    sql("create index table1_index on table table1(id, country) as 'carbondata' properties" +
+        "('sort_scope'='global_sort', 'Global_sort_partitions'='3')")
+    checkAnswerWithoutSort(sql("select id, country from table1_index"),
+      Seq(Row("1", "india"), Row("2", "china")))
+    // check for valid sort_scope
+    checkExistence(sql("describe formatted table1_index"), true, "Sort Scope global_sort")
+    // check the invalid sort scope
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='tim_sort', 'Global_sort_partitions'='3')"))
+      .getMessage
+      .contains("Invalid SORT_SCOPE tim_sort"))
+    // check for invalid global_sort_partitions
+    assert(intercept[MalformedCarbonCommandException](sql(
+      "create index index_2 on table table1(id, country) as 'carbondata' properties" +
+      "('sort_scope'='global_sort', 'Global_sort_partitions'='-1')"))
+      .getMessage
+      .contains("Table property global_sort_partitions : -1 is invalid"))
+    sql("drop index table1_index on table1")

Review comment:
       I can see that from your PR #3608 , you have used t1 as table name and i1 as index name in DropTableTest. is that clean and meaningful name ? I don't want to argue further.
   Table1 is still a table name I have not named it as car or bike.




----------------------------------------------------------------
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]

[GitBox]

akashrn5 commented on pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#issuecomment-690245938


   LGTM


----------------------------------------------------------------
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]

[GitBox]

kunal642 commented on pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787#issuecomment-690252527


   LGTM


----------------------------------------------------------------
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]

[GitBox]

asfgit closed pull request #3787:
URL: https://github.com/apache/carbondata/pull/3787


   


----------------------------------------------------------------
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]