impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5036: Parquet count star optimization
Date Fri, 09 Jun 2017 20:28:00 GMT
Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-5036: Parquet count star optimization

Patch Set 2:

File be/src/exec/

Line 428:     RETURN_IF_ERROR(
> In most cases we're allocating way too much here. We can get a more accurat

Line 437:       int64_t* dst_slot = reinterpret_cast<int64_t*>(dst_tuple->GetSlot(0));
> The dst_tuple->GetSlot(0) looks wrong. There's no guarantee that the target
File fe/src/main/java/org/apache/impala/analysis/

Line 333:       FunctionCallExpr f = (FunctionCallExpr) substitutedAgg;
> remove

Line 359:     //Preconditions.checkState(mergeAggInfo_ == null);
> ?
Looks like I didn't undo all the changes I was making when experimenting. Fixed.
File fe/src/main/java/org/apache/impala/analysis/

Line 571:   public FunctionCallExpr getMergeAggInputFn() { return mergeAggInputFn_; }
> Do we need these getters/setters?
Yes, they are used on line 617, for example. I don't think it would be a good idea to make
mergeAggInputFn_ public.

Line 621:       if (!(substitutedFn instanceof FunctionCallExpr)) return e;
> This looks like an error we should not ignore. Convert into Preconditions c
File fe/src/main/java/org/apache/impala/analysis/

Line 334:     ArrayList<SlotDescriptor> materializedSlots = getMaterializedSlots();
> The previous implementation seemed cheaper, I'd prefer to leave it. Using h
File fe/src/main/java/org/apache/impala/planner/

Line 105:  * TODO: pass in range restrictions.
> Might be good to add a word or two about the agg optimization flow, i.e., t

Line 134:   // Set to true if the count(*) aggregation can be optimized by populating the
tuple with
> Set to true if the query block of this scan has a count(*) aggregation that

Line 238:     // Create two functions that we will put into an smap. We want to replace the
> Integrate this into a function comment. It should describe what this functi

Line 247:     sd.setType(Type.BIGINT);
> set slot as non-nullable

Line 253:     sumFn.analyze(analyzer);
> use analyzeNoThrow() and remove the throws declaration

Line 288:           (getTupleDesc().getMaterializedSlots().isEmpty() ||
> use desc_ instead of getTupleDesc()
File fe/src/main/java/org/apache/impala/planner/

Line 1207:    * If 'hdfsTblRef' only contains partition columns and 'fastPartitionKeyScans'
> comment on new param

Line 1268:    * 'fastPartitionKeyScans' indicates whether to try to produce the slots with
> comment new param

Line 1512:    * 'fastPartitionKeyScans' indicates whether to try to produce the slots with
> comment new param
File fe/src/test/java/org/apache/impala/analysis/

Line 378:     RewritesOk("count(id)", rule, null);
> Also check count(1+1) and count(1+NULL)

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I536b85c014821296aed68a0c68faadae96005e62
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message