impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4192: Disentangle Expr and ExprContext
Date Thu, 01 Jun 2017 15:34:20 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4192: Disentangle Expr and ExprContext

Patch Set 16:


Looking good, just had a handful of comments.
File be/src/exec/

Line 93:     pool_->Add(build_expr);
Wrap pool_->Add() around the above line. Makes it harder to reintroduce the bug by adding
a line in-between.
File be/src/exec/

Line 165:         new RowDescriptor(min_max_tuple_desc_, /* is_nullable */ false));
> Good point. I think heap allocated may be safer in case the expression keep
I think we should be explicit in the ScalarExpr comment about whether Create() can hold onto
a reference, or whether it has  to copy.
File be/src/exec/

PS16, Line 163: 	
File be/src/exec/

Line 87:     Status status = ScalarExpr::Create(eq_join_conjunct.left, child(0)->row_desc(),
It would be nice if we had a way to make this cleanup more automatic to make it harder to
introduce bugs like this. Maybe ScalarExpr::Create() should close the expr and return NULL
if it fails?

Similarly maybe the vector version of ScalarExpr::Create() should close all the exprs and
clear the vector on failure.
File be/src/exprs/anyval-util.h:

Line 32: using impala_udf::FunctionContext;
Using the whole namespace seems fine to me too so long as it's scoped to impala (this is also
File be/src/exprs/scalar-expr-evaluator.h:

Line 157:   /// or its sub-expressions. 'start_idx' and 'end_idx' correspond to the range
> Yes, it'd work out but not sure the difference in overhead for not reusing 
I'm not too concerned about that. There's two possible levels of caching:

* In the MemPool if we don't transfer and call Clear()
* In TCMalloc or the new buffer pool, where there's thread-local caching or core-local caching
respectively. TCMalloc's caching should be pretty effective for small allocations.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefdc9aeeba033355cb9497e3a5d2363627dcf2f3
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message