QiangCai commented on a change in pull request #3778:
URL: https://github.com/apache/carbondata/pull/3778#discussion_r469772276
##########
File path:
core/src/main/java/org/apache/carbondata/core/scan/complextypes/ArrayQueryType.java
##########
@@ -39,7 +39,7 @@ public ArrayQueryType(String name, String parentName, int
columnIndex) {
@Override
public void addChildren(GenericQueryType children) {
- if (this.getName().equals(children.getParentName())) {
+ if (null == this.getName() ||
this.getName().equals(children.getParentName())) {
Review comment:
When the name can be null?
##########
File path:
core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java
##########
@@ -2456,4 +2456,15 @@ private CarbonCommonConstants() {
* property which defines the insert stage flow
*/
public static final String IS_INSERT_STAGE = "is_insert_stage";
+
+ /**
+ * Until the threshold for complex filter is reached, row id will be set to
the bitset in
+ * implicit filter during secondary index pruning
+ */
+ public static final String SI_COMPLEX_FILTER_THRESHOLD =
"carbon.si.complex.filter.threshold";
Review comment:
better to move all constants of SI together to one place of this class.
##########
File path:
core/src/main/java/org/apache/carbondata/core/scan/expression/conditional/ImplicitExpression.java
##########
@@ -41,39 +44,62 @@
* map that contains the mapping of block id to the valid blocklets in that
block which contain
* the data as per the applied filter
*/
- private Map<String, Set<Integer>> blockIdToBlockletIdMapping;
+ private final Map<String, Set<String>> blockIdToBlockletIdMapping;
+
+ /**
+ * checks if implicit filter exceeds complex filter threshold
+ */
+ private boolean isThresholdReached;
public ImplicitExpression(List<Expression> implicitFilterList) {
+ final Logger LOGGER =
LogServiceFactory.getLogService(getClass().getName());
Review comment:
move LOGGER to be a static field of this class
##########
File path:
core/src/main/java/org/apache/carbondata/core/indexstore/blockletindex/BlockletDataRefNode.java
##########
@@ -221,4 +221,9 @@ public int numberOfNodes() {
public List<TableBlockInfo> getBlockInfos() {
Review comment:
after we added getTableBlockInfo(), can we remove this method?
##########
File path:
core/src/main/java/org/apache/carbondata/core/scan/expression/conditional/ImplicitExpression.java
##########
@@ -41,39 +44,62 @@
* map that contains the mapping of block id to the valid blocklets in that
block which contain
* the data as per the applied filter
*/
- private Map<String, Set<Integer>> blockIdToBlockletIdMapping;
+ private final Map<String, Set<String>> blockIdToBlockletIdMapping;
+
+ /**
+ * checks if implicit filter exceeds complex filter threshold
+ */
+ private boolean isThresholdReached;
public ImplicitExpression(List<Expression> implicitFilterList) {
+ final Logger LOGGER =
LogServiceFactory.getLogService(getClass().getName());
// initialize map with half the size of filter list as one block id can
contain
// multiple blocklets
blockIdToBlockletIdMapping = new HashMap<>(implicitFilterList.size() / 2);
for (Expression value : implicitFilterList) {
String blockletPath = ((LiteralExpression)
value).getLiteralExpValue().toString();
addBlockEntry(blockletPath);
}
+ int complexFilterThreshold =
CarbonProperties.getInstance().getComplexFilterThresholdForSI();
+ isThresholdReached = implicitFilterList.size() > complexFilterThreshold;
+ if (isThresholdReached) {
+ LOGGER.info("Implicit Filter Size: " + implicitFilterList.size() + ",
Threshold is: "
+ + complexFilterThreshold);
+ }
}
- public ImplicitExpression(Map<String, Set<Integer>>
blockIdToBlockletIdMapping) {
+ public ImplicitExpression(Map<String, Set<String>>
blockIdToBlockletIdMapping) {
this.blockIdToBlockletIdMapping = blockIdToBlockletIdMapping;
}
private void addBlockEntry(String blockletPath) {
Review comment:
The logic of this method is hard to understand.
Can we add a flag into ImplicitExpression when it is created?
if it is blocklet level, we addBlockletEntry.
if it is row level, we addRowEntry.
##########
File path:
core/src/main/java/org/apache/carbondata/core/scan/complextypes/ArrayQueryType.java
##########
@@ -97,21 +97,31 @@ public void fillRequiredBlockData(RawBlockletColumnChunks
blockChunkHolder)
@Override
public Object getDataBasedOnDataType(ByteBuffer dataBuffer) {
- Object[] data = fillData(dataBuffer);
+ return getDataBasedOnDataType(dataBuffer, false);
+ }
+
+ @Override
+ public Object getDataBasedOnDataType(ByteBuffer dataBuffer, boolean
getBytesData) {
Review comment:
how about to keep the old method and add a new method
getObjectDataBasedOnDataType?
It will not need this boolean parameter.
----------------------------------------------------------------
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
