[GitHub] [carbondata] ShreelekhyaG opened a new pull request #4080: [WIP] Filter query having invalid results when add segment to SI with Indexserver

[GitBox]

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


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


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


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


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



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithAddSegment.scala
##########
@@ -86,8 +86,8 @@ class TestSIWithAddSegment extends QueryTest with BeforeAndAfterAll {
     sql(s"alter table maintable1 add segment options('path'='${ newSegmentPath }', " +
         s"'format'='carbon')")
     sql("CREATE INDEX maintable1_si  on table maintable1 (c) as 'carbondata'")
-    assert(sql("show segments for table maintable1_si").collect().length ==
-           sql("show segments for table maintable1").collect().length)
+    assert(sql("show segments for table maintable1_si").collect().length == 2)
+    assert(sql("show segments for table maintable1").collect().length == 3)

Review comment:
       also have an assert of checking SI table is disabled and query doesn't hit SI

##########
File path: core/src/main/java/org/apache/carbondata/core/indexstore/ExtendedBlocklet.java
##########
@@ -221,7 +223,13 @@ public void deserializeFields(DataInput in, String[] locations, String tablePath
       indexUniqueId = in.readUTF();
     }
     String filePath = getPath();
-    if (filePath.startsWith(File.separator)) {
+    boolean isLocalFile = FileFactory.getCarbonFile(filePath) instanceof LocalCarbonFile;
+    // If it is external segment path, table path need not be appended to filePath
+    // Example filepath: hdfs://hacluster/opt/newsegmentpath/
+    // filePath value would start with hdfs:// or s3:// . If it is local
+    // ubuntu storage, it starts with File separator, so check if given path exists or not.
+    if ((!isLocalFile && filePath.startsWith(File.separator)) || (isLocalFile && !FileFactory

Review comment:
       the comment is not clear, please rewrite it with better example and scenarios




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


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


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


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


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






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



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithAddSegment.scala
##########
@@ -86,8 +86,8 @@ class TestSIWithAddSegment extends QueryTest with BeforeAndAfterAll {
     sql(s"alter table maintable1 add segment options('path'='${ newSegmentPath }', " +
         s"'format'='carbon')")
     sql("CREATE INDEX maintable1_si  on table maintable1 (c) as 'carbondata'")
-    assert(sql("show segments for table maintable1_si").collect().length ==
-           sql("show segments for table maintable1").collect().length)
+    assert(sql("show segments for table maintable1_si").collect().length == 2)
+    assert(sql("show segments for table maintable1").collect().length == 3)

Review comment:
       also have an assert of checking SI table is disabled and query doesn't hit SI

##########
File path: core/src/main/java/org/apache/carbondata/core/indexstore/ExtendedBlocklet.java
##########
@@ -221,7 +223,13 @@ public void deserializeFields(DataInput in, String[] locations, String tablePath
       indexUniqueId = in.readUTF();
     }
     String filePath = getPath();
-    if (filePath.startsWith(File.separator)) {
+    boolean isLocalFile = FileFactory.getCarbonFile(filePath) instanceof LocalCarbonFile;
+    // If it is external segment path, table path need not be appended to filePath
+    // Example filepath: hdfs://hacluster/opt/newsegmentpath/
+    // filePath value would start with hdfs:// or s3:// . If it is local
+    // ubuntu storage, it starts with File separator, so check if given path exists or not.
+    if ((!isLocalFile && filePath.startsWith(File.separator)) || (isLocalFile && !FileFactory

Review comment:
       the comment is not clear, please rewrite it with better example and scenarios




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

ShreelekhyaG commented on a change in pull request #4080:
URL: https://github.com/apache/carbondata/pull/4080#discussion_r569160528



##########
File path: core/src/main/java/org/apache/carbondata/core/indexstore/ExtendedBlocklet.java
##########
@@ -221,7 +223,13 @@ public void deserializeFields(DataInput in, String[] locations, String tablePath
       indexUniqueId = in.readUTF();
     }
     String filePath = getPath();
-    if (filePath.startsWith(File.separator)) {
+    boolean isLocalFile = FileFactory.getCarbonFile(filePath) instanceof LocalCarbonFile;
+    // If it is external segment path, table path need not be appended to filePath
+    // Example filepath: hdfs://hacluster/opt/newsegmentpath/
+    // filePath value would start with hdfs:// or s3:// . If it is local
+    // ubuntu storage, it starts with File separator, so check if given path exists or not.
+    if ((!isLocalFile && filePath.startsWith(File.separator)) || (isLocalFile && !FileFactory

Review comment:
       ok 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]

ShreelekhyaG commented on a change in pull request #4080:
URL: https://github.com/apache/carbondata/pull/4080#discussion_r569160839



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithAddSegment.scala
##########
@@ -86,8 +86,8 @@ class TestSIWithAddSegment extends QueryTest with BeforeAndAfterAll {
     sql(s"alter table maintable1 add segment options('path'='${ newSegmentPath }', " +
         s"'format'='carbon')")
     sql("CREATE INDEX maintable1_si  on table maintable1 (c) as 'carbondata'")
-    assert(sql("show segments for table maintable1_si").collect().length ==
-           sql("show segments for table maintable1").collect().length)
+    assert(sql("show segments for table maintable1_si").collect().length == 2)
+    assert(sql("show segments for table maintable1").collect().length == 3)

Review comment:
       Disabled SI table after alter add load and added a check to verify in test cases.




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


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


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


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


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



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithAddSegment.scala
##########
@@ -86,8 +86,8 @@ class TestSIWithAddSegment extends QueryTest with BeforeAndAfterAll {
     sql(s"alter table maintable1 add segment options('path'='${ newSegmentPath }', " +
         s"'format'='carbon')")
     sql("CREATE INDEX maintable1_si  on table maintable1 (c) as 'carbondata'")
-    assert(sql("show segments for table maintable1_si").collect().length ==
-           sql("show segments for table maintable1").collect().length)
+    assert(sql("show segments for table maintable1_si").collect().length == 2)
+    assert(sql("show segments for table maintable1").collect().length == 3)

Review comment:
       @akashrn5 why we need to disable SI ?




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



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithAddSegment.scala
##########
@@ -86,8 +86,8 @@ class TestSIWithAddSegment extends QueryTest with BeforeAndAfterAll {
     sql(s"alter table maintable1 add segment options('path'='${ newSegmentPath }', " +
         s"'format'='carbon')")
     sql("CREATE INDEX maintable1_si  on table maintable1 (c) as 'carbondata'")
-    assert(sql("show segments for table maintable1_si").collect().length ==
-           sql("show segments for table maintable1").collect().length)
+    assert(sql("show segments for table maintable1_si").collect().length == 2)
+    assert(sql("show segments for table maintable1").collect().length == 3)

Review comment:
       @akashrn5 @ShreelekhyaG  why we need to disable SI ?  I think we no need to disable SI, when external seg is added. Query will use SI to prune transactional segment and main table to prune external segment




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



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithAddSegment.scala
##########
@@ -86,8 +86,8 @@ class TestSIWithAddSegment extends QueryTest with BeforeAndAfterAll {
     sql(s"alter table maintable1 add segment options('path'='${ newSegmentPath }', " +
         s"'format'='carbon')")
     sql("CREATE INDEX maintable1_si  on table maintable1 (c) as 'carbondata'")
-    assert(sql("show segments for table maintable1_si").collect().length ==
-           sql("show segments for table maintable1").collect().length)
+    assert(sql("show segments for table maintable1_si").collect().length == 2)
+    assert(sql("show segments for table maintable1").collect().length == 3)

Review comment:
       since for external added segments we don't load SI, its scenario of segment mismatch of SI and main table, it may lead to wrong results or failure. We don't yet support of getting the results from SI for some segments and some segments from main table 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]

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



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithAddSegment.scala
##########
@@ -86,8 +86,8 @@ class TestSIWithAddSegment extends QueryTest with BeforeAndAfterAll {
     sql(s"alter table maintable1 add segment options('path'='${ newSegmentPath }', " +
         s"'format'='carbon')")
     sql("CREATE INDEX maintable1_si  on table maintable1 (c) as 'carbondata'")
-    assert(sql("show segments for table maintable1_si").collect().length ==
-           sql("show segments for table maintable1").collect().length)
+    assert(sql("show segments for table maintable1_si").collect().length == 2)
+    assert(sql("show segments for table maintable1").collect().length == 3)

Review comment:
       We use externalSegment Resolver for pruning external segments on main table and for remaining segments, we create a implicit filter to prune using SI. So, there will not be any wrong results for the query




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


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


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


   @ShreelekhyaG please update PR description


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


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


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


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


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


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


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