[GitHub] [carbondata] maheshrajus opened a new pull request #3639: [WIP] Secondary Index enable on partition Table

[GitBox]

CarbonDataQA1 commented on issue #3639: [WIP] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#issuecomment-591950341
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2210/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3639: [WIP] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#issuecomment-592065881
 
 
   Build Success with Spark 2.4.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbon_PR_Builder_2.4.4/514/
   

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


With regards,
Apache Git Services

[GitBox]

CarbonDataQA1 commented on issue #3639: [WIP] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#issuecomment-592104285
 
 
   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12545/job/ApacheCarbonPRBuilder2.3/2214/
   

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386006290
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/scan/scanner/impl/BlockletFilterScanner.java
 ##########
 @@ -453,8 +453,8 @@ private BlockletScannedResult executeFilterForPages(
     scannedResult.setPageIdFiltered(pageFilteredPages);
     scannedResult.setLazyBlockletLoader(lazyBlocklet);
     scannedResult.setBlockletId(
-        blockExecutionInfo.getBlockIdString() + CarbonCommonConstants.FILE_SEPARATOR
-            + rawBlockletColumnChunks.getDataBlock().blockletIndex());
+        blockExecutionInfo.getBlockIdString(),
+            "" + rawBlockletColumnChunks.getDataBlock().blockletIndex());
 
 Review comment:
   use String.valueOf instead

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386006297
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/scan/scanner/impl/BlockletFullScanner.java
 ##########
 @@ -83,9 +82,8 @@ public BlockletScannedResult scanBlocklet(
         .get(QueryStatisticsConstants.TOTAL_PAGE_SCANNED);
     totalPagesScanned.addCountStatistic(QueryStatisticsConstants.TOTAL_PAGE_SCANNED,
         totalPagesScanned.getCount() + rawBlockletColumnChunks.getDataBlock().numberOfPages());
-    String blockletId = blockExecutionInfo.getBlockIdString() + CarbonCommonConstants.FILE_SEPARATOR
-        + rawBlockletColumnChunks.getDataBlock().blockletIndex();
-    scannedResult.setBlockletId(blockletId);
+    scannedResult.setBlockletId(blockExecutionInfo.getBlockIdString(),
+        "" + rawBlockletColumnChunks.getDataBlock().blockletIndex());
 
 Review comment:
   use String.valueOf instead

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386006342
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/mutate/data/BlockMappingVO.java
 ##########
 @@ -30,6 +30,8 @@
 
   private Map<String, RowCountDetailsVO> completeBlockRowDetailVO;
 
+  private Map<String, String> blockToSegmentMapping;
 
 Review comment:
   There are too many mappings in this class, can you avoid it. Its meaning is not easy to understand

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


With regards,
Apache Git Services

[GitBox]

jackylk commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386006406
 
 

 ##########
 File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/rdd/CarbonSecondaryIndexRDD.scala
 ##########
 @@ -190,24 +191,23 @@ class CarbonSecondaryIndexRDD[K, V](
     val startTime = System.currentTimeMillis()
     val absoluteTableIdentifier: AbsoluteTableIdentifier = AbsoluteTableIdentifier.from(
       carbonStoreLocation, databaseName, factTableName, tableId)
-    val updateStatusManager: SegmentUpdateStatusManager = new SegmentUpdateStatusManager(
-      carbonLoadModel.getCarbonDataLoadSchema.getCarbonTable)
     val jobConf: JobConf = new JobConf(hadoopConf)
     SparkHadoopUtil.get.addCredentials(jobConf)
     val job: Job = new Job(jobConf)
-    val format = CarbonInputFormatUtil.createCarbonInputFormat(absoluteTableIdentifier, job)
+
+    if (carbonLoadModel.getCarbonDataLoadSchema.getCarbonTable.isHivePartitionTable) {
+      job.getConfiguration
 
 Review comment:
   Why do you set this config, please write comment

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


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386084021
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
 ##########
 @@ -2756,11 +2756,29 @@ private static long getMaxOfBlockAndFileSize(long blockSize, long fileSize) {
    * @param identifier
    * @param filePath
    * @param segmentId
+   * @param isTransactionalTable
    * @param isStandardTable
    * @return
    */
   public static String getBlockId(AbsoluteTableIdentifier identifier, String filePath,
       String segmentId, boolean isTransactionalTable, boolean isStandardTable) {
+    return getBlockId(identifier, filePath, segmentId, isTransactionalTable, isStandardTable,
+        false);
+  }
+
+  /**
+   * Generate the blockid as per the block path
+   *
+   * @param identifier
 
 Review comment:
   remove @param from comment, i not explained and straight forward

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


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386083977
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/statusmanager/SegmentUpdateStatusManager.java
 ##########
 @@ -176,6 +176,10 @@ public SegmentUpdateDetails getDetailsForABlock(String key) {
 
   }
 
+  public Map<String, SegmentUpdateDetails> getBlockAndDetailsMap() {
 
 Review comment:
   please add comment what it returns and what it contains

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


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386084044
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
 ##########
 @@ -2779,10 +2797,18 @@ public static String getBlockId(AbsoluteTableIdentifier identifier, String fileP
         } else {
           partitionDir = "";
         }
-        // Replace / with # on partition director to support multi level partitioning. And access
-        // them all as a single entity.
-        blockId = partitionDir.replace("/", "#") + CarbonCommonConstants.FILE_SEPARATOR
-            + segmentId + CarbonCommonConstants.FILE_SEPARATOR + blockName;
+        if (isPartitionTable) {
+          blockId =
+              partitionDir.replace("/", "#")
 
 Review comment:
   Use CarbonCommonConstants.Fileseparator

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


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386084217
 
 

 ##########
 File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/rdd/CarbonSecondaryIndexRDD.scala
 ##########
 @@ -190,24 +191,23 @@ class CarbonSecondaryIndexRDD[K, V](
     val startTime = System.currentTimeMillis()
     val absoluteTableIdentifier: AbsoluteTableIdentifier = AbsoluteTableIdentifier.from(
       carbonStoreLocation, databaseName, factTableName, tableId)
-    val updateStatusManager: SegmentUpdateStatusManager = new SegmentUpdateStatusManager(
-      carbonLoadModel.getCarbonDataLoadSchema.getCarbonTable)
     val jobConf: JobConf = new JobConf(hadoopConf)
     SparkHadoopUtil.get.addCredentials(jobConf)
     val job: Job = new Job(jobConf)
-    val format = CarbonInputFormatUtil.createCarbonInputFormat(absoluteTableIdentifier, job)
+
+    if (carbonLoadModel.getCarbonDataLoadSchema.getCarbonTable.isHivePartitionTable) {
+      job.getConfiguration
+        .set("current.segmentfile", segmentId + "_" + carbonLoadModel.getFactTimeStamp)
 
 Review comment:
   use CarbonCommonConstants.underscore

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


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386084106
 
 

 ##########
 File path: hadoop/src/main/java/org/apache/carbondata/hadoop/api/CarbonTableInputFormat.java
 ##########
 @@ -188,7 +189,10 @@
 
     List<Segment> segmentToAccess =
         getFilteredSegment(job, validAndInProgressSegments, false, readCommittedScope);
-
+    String segmentFileName = job.getConfiguration().get("current.segmentfile");
 
 Review comment:
   what is current.segmentfile? can we add current.segmentfile as constant?

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


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386084358
 
 

 ##########
 File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/iud/DeleteCarbonTableTestCase.scala
 ##########
 @@ -70,7 +73,7 @@ class DeleteCarbonTableTestCase extends QueryTest with BeforeAndAfterAll {
     sql("""delete from dest where c1 IN ('d', 'e')""").show
     checkAnswer(
       sql("""select c1 from dest"""),
-      Seq(Row("a"), Row("b"),Row("c"))
+      Seq(Row("a"), Row("b"), Row("c"))
 
 Review comment:
   revert

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


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386084353
 
 

 ##########
 File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/iud/DeleteCarbonTableTestCase.scala
 ##########
 @@ -40,17 +40,20 @@ class DeleteCarbonTableTestCase extends QueryTest with BeforeAndAfterAll {
     sql("drop database  if exists iud_db cascade")
     sql("create database  iud_db")
 
-    sql("""create table iud_db.source2 (c11 string,c22 int,c33 string,c55 string, c66 int) STORED AS carbondata""")
+    sql(
+      """create table iud_db.source2 (c11 string,c22 int,c33 string,c55 string, c66 int) STORED
+        |AS carbondata""".stripMargin)
     sql(s"""LOAD DATA LOCAL INPATH '$resourcesPath/IUD/source2.csv' INTO table iud_db.source2""")
     sql("use iud_db")
   }
+
   test("delete data from carbon table with alias [where clause ]") {
     sql("""create table iud_db.dest (c1 string,c2 int,c3 string,c5 string) STORED AS carbondata""")
     sql(s"""LOAD DATA LOCAL INPATH '$resourcesPath/IUD/dest.csv' INTO table iud_db.dest""")
     sql("""delete from iud_db.dest d where d.c1 = 'a'""").show
     checkAnswer(
       sql("""select c2 from iud_db.dest"""),
-      Seq(Row(2), Row(3),Row(4), Row(5))
+      Seq(Row(2), Row(3), Row(4), Row(5))
 
 Review comment:
   revert

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


With regards,
Apache Git Services

[GitBox]

akashrn5 commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386084370
 
 

 ##########
 File path: integration/spark/src/test/scala/org/apache/carbondata/spark/testsuite/iud/DeleteCarbonTableTestCase.scala
 ##########
 @@ -119,18 +122,22 @@ class DeleteCarbonTableTestCase extends QueryTest with BeforeAndAfterAll {
 
   test("partition delete data from carbon table with alias [where clause ]") {
     sql("drop table if exists iud_db.dest")
-    sql("""create table iud_db.dest (c1 string,c2 int,c5 string) PARTITIONED BY(c3 string) STORED AS carbondata""")
+    sql(
+      """create table iud_db.dest (c1 string,c2 int,c5 string) PARTITIONED BY(c3 string) STORED AS
+        |carbondata""".stripMargin)
     sql(s"""LOAD DATA LOCAL INPATH '$resourcesPath/IUD/dest.csv' INTO table iud_db.dest""")
     sql("""delete from iud_db.dest d where d.c1 = 'a'""").show
     checkAnswer(
       sql("""select c2 from iud_db.dest"""),
-      Seq(Row(2), Row(3),Row(4), Row(5))
+      Seq(Row(2), Row(3), Row(4), Row(5))
 
 Review comment:
   revert

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


With regards,
Apache Git Services

[GitBox]

maheshrajus commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386891998
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/scan/scanner/impl/BlockletFilterScanner.java
 ##########
 @@ -453,8 +453,8 @@ private BlockletScannedResult executeFilterForPages(
     scannedResult.setPageIdFiltered(pageFilteredPages);
     scannedResult.setLazyBlockletLoader(lazyBlocklet);
     scannedResult.setBlockletId(
-        blockExecutionInfo.getBlockIdString() + CarbonCommonConstants.FILE_SEPARATOR
-            + rawBlockletColumnChunks.getDataBlock().blockletIndex());
+        blockExecutionInfo.getBlockIdString(),
+            "" + rawBlockletColumnChunks.getDataBlock().blockletIndex());
 
 Review comment:
   Fixed

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


With regards,
Apache Git Services

[GitBox]

maheshrajus commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386892338
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/scan/scanner/impl/BlockletFullScanner.java
 ##########
 @@ -83,9 +82,8 @@ public BlockletScannedResult scanBlocklet(
         .get(QueryStatisticsConstants.TOTAL_PAGE_SCANNED);
     totalPagesScanned.addCountStatistic(QueryStatisticsConstants.TOTAL_PAGE_SCANNED,
         totalPagesScanned.getCount() + rawBlockletColumnChunks.getDataBlock().numberOfPages());
-    String blockletId = blockExecutionInfo.getBlockIdString() + CarbonCommonConstants.FILE_SEPARATOR
-        + rawBlockletColumnChunks.getDataBlock().blockletIndex();
-    scannedResult.setBlockletId(blockletId);
+    scannedResult.setBlockletId(blockExecutionInfo.getBlockIdString(),
+        "" + rawBlockletColumnChunks.getDataBlock().blockletIndex());
 
 Review comment:
   Fixed

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


With regards,
Apache Git Services

[GitBox]

maheshrajus commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386894958
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
 ##########
 @@ -2756,11 +2756,29 @@ private static long getMaxOfBlockAndFileSize(long blockSize, long fileSize) {
    * @param identifier
    * @param filePath
    * @param segmentId
+   * @param isTransactionalTable
    * @param isStandardTable
    * @return
    */
   public static String getBlockId(AbsoluteTableIdentifier identifier, String filePath,
       String segmentId, boolean isTransactionalTable, boolean isStandardTable) {
+    return getBlockId(identifier, filePath, segmentId, isTransactionalTable, isStandardTable,
+        false);
+  }
+
+  /**
+   * Generate the blockid as per the block path
+   *
+   * @param identifier
 
 Review comment:
   Fixed

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


With regards,
Apache Git Services

[GitBox]

maheshrajus commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386896310
 
 

 ##########
 File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
 ##########
 @@ -2779,10 +2797,18 @@ public static String getBlockId(AbsoluteTableIdentifier identifier, String fileP
         } else {
           partitionDir = "";
         }
-        // Replace / with # on partition director to support multi level partitioning. And access
-        // them all as a single entity.
-        blockId = partitionDir.replace("/", "#") + CarbonCommonConstants.FILE_SEPARATOR
-            + segmentId + CarbonCommonConstants.FILE_SEPARATOR + blockName;
+        if (isPartitionTable) {
+          blockId =
+              partitionDir.replace("/", "#")
 
 Review comment:
   fixed

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


With regards,
Apache Git Services

[GitBox]

maheshrajus commented on a change in pull request #3639: [CARBONDATA-3724] Secondary Index enable on partition Table
URL: https://github.com/apache/carbondata/pull/3639#discussion_r386897897
 
 

 ##########
 File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/rdd/CarbonSecondaryIndexRDD.scala
 ##########
 @@ -190,24 +191,23 @@ class CarbonSecondaryIndexRDD[K, V](
     val startTime = System.currentTimeMillis()
     val absoluteTableIdentifier: AbsoluteTableIdentifier = AbsoluteTableIdentifier.from(
       carbonStoreLocation, databaseName, factTableName, tableId)
-    val updateStatusManager: SegmentUpdateStatusManager = new SegmentUpdateStatusManager(
-      carbonLoadModel.getCarbonDataLoadSchema.getCarbonTable)
     val jobConf: JobConf = new JobConf(hadoopConf)
     SparkHadoopUtil.get.addCredentials(jobConf)
     val job: Job = new Job(jobConf)
-    val format = CarbonInputFormatUtil.createCarbonInputFormat(absoluteTableIdentifier, job)
+
+    if (carbonLoadModel.getCarbonDataLoadSchema.getCarbonTable.isHivePartitionTable) {
+      job.getConfiguration
+        .set("current.segmentfile", segmentId + "_" + carbonLoadModel.getFactTimeStamp)
 
 Review comment:
   Fixed

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


With regards,
Apache Git Services