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