[GitHub] [carbondata] Indhumathi27 opened a new pull request #4106: [WIP] Fix re-arrange schema in logical relation on MV partition table having sort column

[GitBox]

Indhumathi27 opened a new pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106


   
    ### Why is this PR needed?
   
   
    ### What changes were proposed in this PR?
   
       
    ### Does this PR introduce any user interface change?
    - No
    - Yes. (please explain the change and update document)
   
    ### Is any new testcase added?
    - No
    - 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]

[GitBox]

CarbonDataQA2 commented on pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-797474167


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


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

CarbonDataQA2 commented on pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-797478179


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


----------------------------------------------------------------
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 #4106:
URL: https://github.com/apache/carbondata/pull/4106#discussion_r595752950



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertIntoCommand.scala
##########
@@ -181,7 +183,13 @@ case class CarbonInsertIntoCommand(databaseNameOp: Option[String],
       if (isNotReArranged) {
         // Re-arrange the catalog table schema and output for partition relation
         logicalPartitionRelation =
-          getReArrangedSchemaLogicalRelation(reArrangedIndex, logicalPartitionRelation)
+          if (carbonLoadModel.getCarbonDataLoadSchema.getCarbonTable.isMV) {
+            // For MV partition table, partition columns will be at the end. Re-arrange

Review comment:
       as discussed, please add a comment here with example, so reviewers and developers will be clear why only for MV we need to handle it separately




----------------------------------------------------------------
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 #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-800846101


   @Indhumathi27 please change description with example as discussed


----------------------------------------------------------------
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 #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-800846438


   LGTM, @ajantha-bhat please review this once as you have worked on rearrange logic in insert optimization. Please see if there is any impact.


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

CarbonDataQA2 commented on pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-800952347


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


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

CarbonDataQA2 commented on pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-800954845


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


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

Indhumathi27 commented on a change in pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#discussion_r596555566



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/execution/command/management/CarbonInsertIntoCommand.scala
##########
@@ -181,7 +183,13 @@ case class CarbonInsertIntoCommand(databaseNameOp: Option[String],
       if (isNotReArranged) {
         // Re-arrange the catalog table schema and output for partition relation
         logicalPartitionRelation =
-          getReArrangedSchemaLogicalRelation(reArrangedIndex, logicalPartitionRelation)
+          if (carbonLoadModel.getCarbonDataLoadSchema.getCarbonTable.isMV) {
+            // For MV partition table, partition columns will be at the end. Re-arrange

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]

[GitBox]

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



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/view/rewrite/TestPartitionWithMV.scala
##########
@@ -748,6 +748,22 @@ class TestPartitionWithMV extends QueryTest with BeforeAndAfterAll with BeforeAn
     sql("drop table if exists partitionone")
   }
 
+  test("test partition on MV with sort column") {
+    sql("drop table if exists partitionone")
+    sql("create table if not exists partitionone (ts timestamp, " +
+        "metric STRING, tags_id STRING, value DOUBLE) partitioned by (ts1 timestamp,ts2 timestamp) stored as carbondata TBLPROPERTIES ('SORT_COLUMNS'='metric,ts2')")

Review comment:
       I think above MV rearrange code was added when partition column is not a sort column case I guess.
   
   can you please add one more test case, same but just remove sort columns from the create table and test 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]

[GitBox]

Indhumathi27 commented on a change in pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#discussion_r596564072



##########
File path: integration/spark/src/test/scala/org/apache/carbondata/view/rewrite/TestPartitionWithMV.scala
##########
@@ -748,6 +748,22 @@ class TestPartitionWithMV extends QueryTest with BeforeAndAfterAll with BeforeAn
     sql("drop table if exists partitionone")
   }
 
+  test("test partition on MV with sort column") {
+    sql("drop table if exists partitionone")
+    sql("create table if not exists partitionone (ts timestamp, " +
+        "metric STRING, tags_id STRING, value DOUBLE) partitioned by (ts1 timestamp,ts2 timestamp) stored as carbondata TBLPROPERTIES ('SORT_COLUMNS'='metric,ts2')")

Review comment:
       All other testcase in TestPartitionWithMV are without sort column only and also tested manually. It works




----------------------------------------------------------------
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 pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-801641696


   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]

Indhumathi27 commented on pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-801976661


   retest this please


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

CarbonDataQA2 commented on pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-802113825


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


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

CarbonDataQA2 commented on pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-802123207


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


--
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 #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-802806840


   retest this please


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

CarbonDataQA2 commented on pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-802896732


   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3839/
   


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

CarbonDataQA2 commented on pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-802897202


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


--
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 #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-802914033


   retest this please


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

CarbonDataQA2 commented on pull request #4106:
URL: https://github.com/apache/carbondata/pull/4106#issuecomment-802995990


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


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