Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4053: Address follow up comments for IMPALA-3610

Patch Set 3:

File be/src/runtime/

Line 374
> what's the rationale for leaving this here?
The query_mem_tracker_ is accessed by the QueryExecState even after TearDown() 
is called to release it in QueryExecState::ClearResultsCache()

Line 264:     is_local = tFilterTarget.is_local_target;
> more interesting: how is the discarded or disabled state recorded in this s
There is no specific state to denote that a filter has been discarded. It 
happens as a consequence of other things, i.e. query completion, agg completion 
or being disabled.
I've updated how disabled state is recorded in this struct.

Line 277: /// it is disabled.
> FilterTarget isn't referenced in the .h file, also move it here

Line 290:   vector<FilterTarget>* targets() { return &targets_; }
> don't comment on the intention of the caller. describe the externally visib

Line 295:   const TRuntimeFilterDesc& desc() const { return desc_; }
> describe externally visible behavior and role of parameters. just looking a
In the new patch the only externally available behavior is the mem tracker 
being updated and the rpc_params being set.

Line 304:   /// Releases tracked memory and frees 'bloom_filter'. A discarded 
filter consumes no
> This does not merely look like an index. Can you comment on why we're using
As opposed to? These are indices to fragment_instance_states_.

Line 733:       partition_filter.push_back(target.is_bound_by_partition_columns 
? "true" : "false");
> this line would need changing if we move pending_count into disabled()
See comment below.

PS2, Line 2069: < "Filter received before routing table complete";
> These two mostly occur together. Maybe move the pending_count check into di
They both mean different things, so moving the pending_count() check into 
disabled() would hamper readability (even though they currently could 
functionally be merged).

Line 2075:   unordered_set<int32_t> target_fragment_instance_idxs;
> remove blank line

Line 2077:     lock_guard<SpinLock> l(filter_lock_);
> and this. this section of code deals with applying updates, so it's a good 

PS2, Line 2078: t = filter_routing_tab
> Is it correct to check for !state->disabled() here? Can it happen that betw
Yes, it can get disabled in ApplyUpdate()

Line 2082:     }
> const FilterTarget&

Line 2090:     //     filters that can't affect the aggregated global filter.
> i find the control flow harder to follow than before, and the code has more
We didn't need to TryConsume() memory for a broadcast filter update as the 
input filter can just be passed on as the output filter. That's the reason for 
the extra special casing. However, even if I do change it back to how it was 
originally, we need to add special cases elsewhere to take care of the 
following case:

We don't want to disable the filter if the first update for a broadcast filter 
hits OOM in ApplyUpdate(), as subsequent updates may arrive at times of lower 
memory pressure.

This requires changing how we track pending_counts for broadcast filters, 
adding a couple of special cases in ApplyUpdate() and we don't get the benefit 
of avoiding using extra memory.

I've made the change describing the paragraph above, so you can see how it 
looks and it'll be easier to decide on what to do about this special casing.

PS2, Line 2092:   * if the filter could not be allocated
> How about "Filter has been processed and distributed completely, and can be
The distribution happens after this, so I've left that part out. Changed the 
rest of it.

PS2, Line 2147:     // If it's a broadcast filter, future updates may arrive at 
a time of lower memory
              :       // pressure, so do not disable.
> Could this go into Discard()?
It could but it would make the readability a little harder. This is in a way a 
matching call to L2111.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7499558cc9d3622a9f3b68904aa5fe92a113f579
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

Reply via email to