impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Date Sat, 21 Jan 2017 02:46:02 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen

Patch Set 1:

File be/src/codegen/

PS1, Line 119: Value
> Function and GlobalVariable are both subclasses of GlobalObject so it would

PS1, Line 124: getParent()->getParent()
> getFunction()?

PS1, Line 149: fn_name
> Is this right? It doesn't reference any variables that are updated within t
For global variables, we only count those functions not defined in Impalad binary.

Line 382:   for (const string& gv: gv_with_fns_) {
> This is the only place this is used, so we could instead just directly stor
File be/src/codegen/llvm-codegen.h:

PS1, Line 511: FindUses
> FindGlobalUses?
Renamed to FindGlobalUsers();

Line 516:   static void FillCalleeMap(llvm::Function* fn, CalleeMap& callee_map,
> The name and comment should communicate that this excludes functions define
This function is removed in the new patch.
File be/src/exprs/

Line 57
> It seems like we want to be able to inline the constants into the timestamp
Without defining MIN_YEAR as a compilation constant, there isn't much to be inlined.

This new patch sets MIN_YEAR to value 1400 and adds a DCHECK() to verify it matches Date(min_date_year).year().
Inspected the compiled code to verify these class static variables are global constants in
File be/src/runtime/exec-env.h:

Line 115:   Status InitForBeTests();
> I think we should avoid replicating the InitForFeTests() pattern - it's kin

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message