[GitHub] [carbondata] akashrn5 commented on a change in pull request #3748: [CARBONDATA-3801][CARBONDATA-3805][CARBONDATA-3809] Query on partition table with SI having multiple partition columns gives empty results

[GitBox <gi...@apache.org>]

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



##########
File path: core/src/main/java/org/apache/carbondata/core/datastore/block/SegmentPropertiesAndSchemaHolder.java
##########
@@ -297,9 +298,11 @@ public void addSegmentId(int segmentPropertiesIndex, String segmentId) {
     private CarbonRowSchema[] fileFooterEntrySchemaForBlock;
     private CarbonRowSchema[] fileFooterEntrySchemaForBlocklet;
 
-    public SegmentPropertiesWrapper(CarbonTable carbonTable, List<ColumnSchema> columnsInTable) {
+    public SegmentPropertiesWrapper(CarbonTable carbonTable, List<ColumnSchema> columnsInTable,
+        String segmentId) {

Review comment:
       I think this change is not required, because, for the partition table which is transactional table, segment id does not matter. This change will lead to unnecessary  changes. you are passing this just to pass the segment Id to getBlockId method. But if you see the method, for transactional partition table flow segment id not required to get the blockID, so please remove this change and can just pass empty string to getBlockId method.

##########
File path: core/src/main/java/org/apache/carbondata/core/indexstore/blockletindex/BlockIndex.java
##########
@@ -793,9 +794,14 @@ private boolean addBlockBasedOnMinMaxValue(FilterExecuter filterExecuter, byte[]
     BitSet bitSet = null;
     if (filterExecuter instanceof ImplicitColumnFilterExecutor) {
       String uniqueBlockPath;
-      if (segmentPropertiesWrapper.getCarbonTable().isHivePartitionTable()) {
-        uniqueBlockPath = filePath
-            .substring(segmentPropertiesWrapper.getCarbonTable().getTablePath().length() + 1);
+      String blockId = null;
+      CarbonTable carbonTable = segmentPropertiesWrapper.getCarbonTable();
+      if (carbonTable.isHivePartitionTable()) {
+        uniqueBlockPath = filePath.substring(carbonTable.getTablePath().length() + 1);
+        boolean isStandardTable = CarbonUtil.isStandardCarbonTable(carbonTable);
+        blockId = CarbonUtil.getBlockId(carbonTable.getAbsoluteTableIdentifier(), filePath,
+            segmentPropertiesWrapper.getSegmentId(), carbonTable.isTransactionalTable(),
+            isStandardTable, carbonTable.isHivePartitionTable());

Review comment:
       here better to get the blockid and assign to uniqueBlockPath, which will take care later in query flow to find shortblockid, no need to calculate here and pass ahead which will make us to change all the interfaces. 

##########
File path: core/src/main/java/org/apache/carbondata/core/scan/filter/executer/ImplicitColumnFilterExecutor.java
##########
@@ -35,7 +35,7 @@
    * @return
    */
   BitSet isFilterValuesPresentInBlockOrBlocklet(byte[][] maxValue, byte[][] minValue,
-      String uniqueBlockPath, boolean[] isMinMaxSet);
+      String uniqueBlockPath, boolean[] isMinMaxSet, String shortBlockId);

Review comment:
       So once after the above comment fix, all the interface changes can be removed.

##########
File path: core/src/test/java/org/apache/carbondata/core/indexstore/blockletindex/TestBlockletIndex.java
##########
@@ -54,7 +54,7 @@
 
     new MockUp<ImplicitIncludeFilterExecutorImpl>() {
       @Mock BitSet isFilterValuesPresentInBlockOrBlocklet(byte[][] maxValue, byte[][] minValue,
-          String uniqueBlockPath, boolean[] isMinMaxSet) {
+          String uniqueBlockPath, boolean[] isMinMaxSet, String shortBlockId) {

Review comment:
       revert these




----------------------------------------------------------------
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:
users@infra.apache.org