impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Date Wed, 18 Jan 2017 22:43:03 GMT
Tim Armstrong has posted comments on this change.

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


Patch Set 1:

(8 comments)

The general approach looks good - mainly the comments are trying to make it easier to understand
how the different functions and data structures fit together.

http://gerrit.cloudera.org:8080/#/c/5732/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

PS1, Line 119: Value
Function and GlobalVariable are both subclasses of GlobalObject so it would clearer if this
was vector<GlobalObject*>. Maybe it could be called 'global_users' or something like
that?


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


PS1, Line 149: fn_name
Is this right? It doesn't reference any variables that are updated within the loop.


Line 382:   for (const string& gv: gv_with_fns_) {
This is the only place this is used, so we could instead just directly store the set of functions
to be materialised. I think it would be easier to understand the purpose of a set called 'always_materialize_fns_'
or something along those lines.


http://gerrit.cloudera.org:8080/#/c/5732/1/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

PS1, Line 511: FindUses
FindGlobalUses?


Line 516:   static void FillCalleeMap(llvm::Function* fn, CalleeMap& callee_map,
The name and comment should communicate that this excludes functions defined in the impalad
daemon.


http://gerrit.cloudera.org:8080/#/c/5732/1/be/src/exprs/timestamp-functions-ir.cc
File be/src/exprs/timestamp-functions-ir.cc:

Line 57
It seems like we want to be able to inline the constants into the timestamp functions below.
Maybe move the constant values into the header so they're visible? I.e. have 

const int64_t MAX_YEAR = 10000; in the header 

and 

const int64_t TimestampFunctions::MAX_YEAR; in timestamp-functions.cc


http://gerrit.cloudera.org:8080/#/c/5732/1/be/src/runtime/exec-env.h
File be/src/runtime/exec-env.h:

Line 115:   Status InitForBeTests();
I think we should avoid replicating the InitForFeTests() pattern - it's kind of confusing.
It's really not obvious that InitForBeTests() should force codegen in that place.

It would probably make more sense to have a static flag in FeSupport that specifically forces
codegen - then it's easier to understand why codegen is enabled for expr-test.


-- 
To view, visit http://gerrit.cloudera.org:8080/5732
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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

Mime
View raw message