You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2017/01/18 20:04:03 UTC
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Michael Ho has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5732
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
This change fixes expr-test.cc to work with codegen again.
Fixing it uncovers a couple of bugs which are fixed in this
patch too.
IMPALA-4705: When an IR function is materialized, its
function body is parsed to find all its callee functions
to be materialized too. However, the old code doesn't
detect callee fnctions referenced indirectly (e.g. a
callee function passed as argument to another function).
This change fixes the problem above inspecting the use
lists of llvm::Function objects. When parsing the bitcode
module into memory, LLVM already establishes a use list
for each llvm::Value object which llvm::Function is a
subclass of. A use list contains all the locations in
the module in which the Value is referenced. For a
llvm::Function object, that would be its call sites and
constant expressions referencing the functions. By using
the use lists of llvm::Function in the module, a global
map is established at Impala initialization time to map
functions to their corresponding callee functions. This
map is then used when materializing a function to ensure
all its callee functions are also materialized recursively.
IMPALA-4779: conditional function isfalse(), istrue(),
isnotfalse(), isnotrue() aren't cross-compiled so they
will lead to unexpected query failure when codegen is enabled.
This change will cross-compile these functions.
IMPALA-4780: next_day() always returns NULL when codegen
is enabled. The bound checks for next_day() use some class
static variables initialized in the global constructors
(@llvm.global_ctors). However, we never execute the global
constructors before calling the JIT compiled functions.
This causes these variables to remain as zero, causing all
executions of next_day() to fail the bound checks. This
change fixes the problem above by not cross-compiling the
initialization of these class static variables so they are
treated as externally defined symbols from LLVM perspectives.
Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timestamp-functions.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/lib-cache.cc
M be/src/service/fe-support.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/test-udfs.cc
M testdata/workloads/functional-query/queries/QueryTest/udf.test
M tests/query_test/test_udfs.py
18 files changed, 247 insertions(+), 213 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/5732/1
--
To view, visit http://gerrit.cloudera.org:8080/5732
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 8: Verified+1
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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:
(8 comments)
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
Done
PS1, Line 124: getParent()->getParent()
> getFunction()?
Done
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
Done
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?
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.
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
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 IR.
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 kin
Done
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 8: Code-Review+2
Carry +2 forward.
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 4:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/5732/4//COMMIT_MSG
Commit Message:
Line 50: defined in the boost library.
is there some verification code we can add to ensure that this problem won't happen again (or isn't happening elsewhere)?
http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
PS4, Line 326: which are also defined in the main module
this doesn't seem to match what the code is doing. Isn't it materializing functions that are referenced by functions of the new module?
But why is this needed? why don't they get materialized when we follow the chain of references in MaterializeFunctionHelper()?
http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
PS4, Line 85: their callee functions
what is a "callee" for a global variable? do you mean reference?
If so, how about:
// Map from global variables/functions to the functions that they reference.
PS4, Line 86: CalleeMap
again, callee sounds to me like only functions would be keys in this map. FunctionRefsMap?
PS4, Line 160: f 'eager' is false, the
: /// functions in the module are not materialized.
this seems to contradict the class header comment that says "llvm::Function objects in the module are materialized lazily". i.e. the behavior you are documented is the expected behavior, no?
So are you adding 'eager' so that the default behavior can be overridden? if so, it should say something like:
If 'eager' is true, the functions are materalized. Otherwise they are materialized lazily via GetFunction().
And if that's what's going on, why not call the parameter 'materialize"?
But, as a caller, how do I choose whether I'm suppose to materialize or not at this point? The need for this seems to contradict the whole explanation about lazy materialization in the header comment.
Finally, rather than threading through this parameter, why not just have the caller do module->MaterializeAll() with the returned module?
PS4, Line 617: which also need to be materialized if
: /// the input global variable or function is materialized
is this really specific to materialization? wouldn't it be enough to just say:
... all the functions referenced by it.
The materialization code itself can explain (or be self documenting) about why references need to be followed when materializing.
PS4, Line 625: fns_to_materialize_
why this name change? does this now store functions that need to be materialized for various reasons? don't we materialize functions that aren't in this set?
http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
since this takes so long to execute, should we either move expr-test.cc to run only in exhaustive, or run only codegen tests when exhaustive?
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 8:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/233/
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5732
to look at the new patch set (#5).
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
This change fixes expr-test.cc to work with codegen as it's
originally intended. Fixing it uncovers a couple of bugs fixed
in this patch:
IMPALA-4705: When an IR function is materialized, its
function body is parsed to find all its callee functions
to be materialized too. However, the old code doesn't
detect callee fnctions referenced indirectly (e.g. a
callee function passed as argument to another function).
This change fixes the problem above inspecting the use
lists of llvm::Function objects. When parsing the bitcode
module into memory, LLVM already establishes a use list
for each llvm::Value object which llvm::Function is a
subclass of. A use list contains all the locations in
the module in which the Value is referenced. For a
llvm::Function object, that would be its call sites and
constant expressions referencing the functions. By using
the use lists of llvm::Function in the module, a global
map is established at Impala initialization time to map
functions to their corresponding callee functions. This
map is then used when materializing a function to ensure
all its callee functions are also materialized recursively.
IMPALA-4779: conditional function isfalse(), istrue(),
isnotfalse(), isnotrue() aren't cross-compiled so they
will lead to unexpected query failure when codegen is enabled.
This change will cross-compile these functions.
IMPALA-4780: next_day() always returns NULL when codegen
is enabled. The bound checks for next_day() use some class
static variables initialized in the global constructors
(@llvm.global_ctors). However, we never execute the global
constructors before calling the JIT compiled functions.
This causes these variables to remain as zero, causing all
executions of next_day() to fail the bound checks. The reason
why these class static variables aren't compiled as global
constants in LLVM IR is that TimestampFunctions::MIN_YEAR is
not a compile time constant. This change fixes the problem
above by setting TimestampFunctions::MIN_YEAR to a known constant
value. A DCHECK is added to verify that it matches the value
defined in the boost library.
Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timestamp-functions.h
M be/src/service/fe-support.cc
M be/src/service/fe-support.h
M be/src/service/impalad-main.cc
M be/src/testutil/test-udfs.cc
M testdata/workloads/functional-query/queries/QueryTest/udf.test
M tests/query_test/test_udfs.py
16 files changed, 221 insertions(+), 232 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/5732/5
--
To view, visit http://gerrit.cloudera.org:8080/5732
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 4:
I think it may have been accidentally disabled by the single node optimisation
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#2).
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
This change fixes expr-test.cc to work with codegen as it's
originally intended. Fixing it uncovers a couple of bugs fixed
in this patch:
IMPALA-4705: When an IR function is materialized, its
function body is parsed to find all its callee functions
to be materialized too. However, the old code doesn't
detect callee fnctions referenced indirectly (e.g. a
callee function passed as argument to another function).
This change fixes the problem above inspecting the use
lists of llvm::Function objects. When parsing the bitcode
module into memory, LLVM already establishes a use list
for each llvm::Value object which llvm::Function is a
subclass of. A use list contains all the locations in
the module in which the Value is referenced. For a
llvm::Function object, that would be its call sites and
constant expressions referencing the functions. By using
the use lists of llvm::Function in the module, a global
map is established at Impala initialization time to map
functions to their corresponding callee functions. This
map is then used when materializing a function to ensure
all its callee functions are also materialized recursively.
IMPALA-4779: conditional function isfalse(), istrue(),
isnotfalse(), isnotrue() aren't cross-compiled so they
will lead to unexpected query failure when codegen is enabled.
This change will cross-compile these functions.
IMPALA-4780: next_day() always returns NULL when codegen
is enabled. The bound checks for next_day() use some class
static variables initialized in the global constructors
(@llvm.global_ctors). However, we never execute the global
constructors before calling the JIT compiled functions.
This causes these variables to remain as zero, causing all
executions of next_day() to fail the bound checks. The reason
why these class static variables aren't compiled as global
constants in LLVM IR is that TimestampFunctions::MIN_YEAR is
not a compile time constant. This change fixes the problem
above by setting TimestampFunctions::MIN_YEAR to a known constant
value. A DCHECK is added to verify that it matches the value
defined in the boost library.
Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timestamp-functions.h
M be/src/runtime/exec-env.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/service/fe-support.cc
M be/src/testutil/test-udfs.cc
M testdata/workloads/functional-query/queries/QueryTest/udf.test
M tests/query_test/test_udfs.py
17 files changed, 200 insertions(+), 188 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/5732/2
--
To view, visit http://gerrit.cloudera.org:8080/5732
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5732/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 194: if (IsDefinedInImpalad(fn_name)) continue;
> I still don't entirely understand this. I understand why we don't want to m
The intention is for the 'callee_map_' to only have functions which need to be materialized when a certain llvm::GlobalObject is referenced. In theory, we don't really need to store entries for global variables but I figure it's more consistent to have both.
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5732/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 194: if (IsDefinedInImpalad(fn_name)) continue;
> Sounds reasonable - I think we just need some comments explaining it
Done
http://gerrit.cloudera.org:8080/#/c/5732/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 63: DECLARE_bool(fe_support_disable_codegen);
> I don't think we want to expose this as a command-line option, do we?
Moved to a static flag in fe-support.cc and it's initialized via InitFeSupport().
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
> I agree it probably doesn't matter too much, but we should generally just p
Done
http://gerrit.cloudera.org:8080/#/c/5732/2/be/src/exprs/timestamp-functions.h
File be/src/exprs/timestamp-functions.h:
Line 42: static const int64_t MAX_YEAR_INTERVAL;
> I think we should define the constant values here, so that they can be inli
Done. Btw, the old definition of MAX_MINUTE_INTERVAL is smaller than it should be. The new patch has the right value.
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 4:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/5732/4//COMMIT_MSG
Commit Message:
Line 50: defined in the boost library.
> is there some verification code we can add to ensure that this problem won'
Not sure if there is a easy way to verify it without parsing the global ctor IR code. I manually inspected the global ctor after this change and the remaining logic seems to be doing:
1. setting up some global variables which aren't really referenced anywhere in the IR code:
_ZN5boost6systemL14posix_categoryE, _ZN5boost6systemL10errno_ecatE and _ZN5boost6systemL11native_ecatE
2. Calling std::ios_base::Init() which is not necessary for IR code to function.
http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
PS4, Line 326: which are also defined in the main module
> this doesn't seem to match what the code is doing. Isn't it materializing
They are not necessarily just function declarations in the new module. They could also be duplicated function definition in both the main module and new module.
In an earlier attempt, this patch eagerly materializes the new module and creates a map of functions referenced by the global variables and functions in the new module. The problem is that for duplicated definition, the linker will pick one of the definition. So after linking, it's unclear which map to use: the function could be from the main module or it could be from the new module. So, to be safe, we need to call MaterializeFunctionHelper with the maps of the main module and the new module. As the function from new module may not linked in and its referenced functions can also become dead code after linking, it leads to some confusing behavior: it's hard to distinguish between an error (i.e. a referenced function is not found in the module after linking) and eliminated dead code.
Ideally, we should record the functions in the new module before linking and compute their referenced functions only linking. The problem is that the "uses" of lvm::Function are only populated after the module has been completely materialized (there is an assert in LLVM for that) so it's not quite feasible.
This patch implements a middle ground approach: materialize any functions which are defined in the main module and also exist either as a declaration or definition in the new module.
http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
PS4, Line 85: their callee functions
> what is a "callee" for a global variable? do you mean reference?
Done
PS4, Line 86: CalleeMap
> again, callee sounds to me like only functions would be keys in this map.
FnRefsMap
PS4, Line 160: f 'eager' is false, the
: /// functions in the module are not materialized.
> this seems to contradict the class header comment that says "llvm::Function
Done. I moved the materialization logic out of CreateFromFile/Memory() and removed the 'eager' argument. This function always materializes the bitcode lazily.
PS4, Line 617: which also need to be materialized if
: /// the input global variable or function is materialized
> is this really specific to materialization? wouldn't it be enough to just s
Done
PS4, Line 625: fns_to_materialize_
> why this name change? does this now store functions that need to be materi
The previous name is not quite self-explanatory. How about fns_to_always_materialize_; ?
http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
> since this takes so long to execute, should we either move expr-test.cc to
I don't feel comfortable testing the codegen path only in the exhaustive builds. The reasoning is that codegen is enabled in Impala by default so our default testing configuration should also be testing the codegen path. GVO is also not run in exhaustive mode so I am worried that bugs may slip through and we won't notice until much later.
http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/exprs/timestamp-functions.h
File be/src/exprs/timestamp-functions.h:
Line 44: /// with its defined max year. date(max_date_time).year() will give 9999 but testing
> I missed updating MAX_YEAR to 9999 in my patch 69859bddfb058d742cece7d7c2e
Done. Also added a DCHECK for that.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 6:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/5732/5/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
PS5, Line 328: in case they
: // are chosen by the linker or referenced by functions in the new module. Note tha
> Okay. How about clarifying:
Done
http://gerrit.cloudera.org:8080/#/c/5732/6/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 172: RETURN_IF_ERROR(LlvmCodeGen::CreateFromMemory(&init_pool, NULL, "init", &init_codegen));
> how about a comment then:
Done
PS6, Line 195: only
> this "only" doesn't make sense, because you also include functions that don
Done
PS6, Line 195: by
> delete double "by"
Done
PS6, Line 198: refereced
> spelling and period not comma at end.
Done
http://gerrit.cloudera.org:8080/#/c/5732/6/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
PS6, Line 85: global variables'/functions'
> function names
Done
PS6, Line 146: libbackend
> is this referring to libfesupport.so?
Done
Line 616: /// returns the set of names of all functions referenced by it.
> Now that this only stores the call graph, how about just explaining it in t
Done
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/exprs/timestamp-functions.h
File be/src/exprs/timestamp-functions.h:
Line 44: /// with its defined max year. date(max_date_time).year() will give 9999 but testing
I missed updating MAX_YEAR to 9999 in my patch 69859bddfb058d742cece7d7c2e3e3f07f29bb6a - this comment about boost being inconsistent no longer applies.
I can also do this in a separate patch but it might be easiest just to fix it now.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 6: Code-Review+2
(8 comments)
Thanks, that's easier to follow. Just some more comment clarification suggestions.
http://gerrit.cloudera.org:8080/#/c/5732/5/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
PS5, Line 328: in case they
: // are chosen by the linker or referenced by functions in the new module. Note tha
> linkModule() will materialize function.
Okay. How about clarifying:
Note that linkModule() will materialize the functions from the new module.
http://gerrit.cloudera.org:8080/#/c/5732/6/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 172: RETURN_IF_ERROR(LlvmCodeGen::CreateFromMemory(&init_pool, NULL, "init", &init_codegen));
how about a comment then:
// LLVM constructs "uses" lists only when materializing.
PS6, Line 195: only
this "only" doesn't make sense, because you also include functions that don't have native versions.
PS6, Line 195: by
delete double "by"
PS6, Line 198: refereced
spelling and period not comma at end.
http://gerrit.cloudera.org:8080/#/c/5732/6/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
PS6, Line 85: global variables'/functions'
function names
(no longer variables).
PS6, Line 146: libbackend
is this referring to libfesupport.so?
Line 616: /// returns the set of names of all functions referenced by it.
Now that this only stores the call graph, how about just explaining it in terms of that:
The call-graph for IR functions within the main module. Used to determine dependencies while materializing functions.
Or something like that.
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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:
Quite a bit longer. 5~6 mins in debug builds on my machine. I think it's worth it given that it covers pretty much all the built-in functions.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#3).
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
This change fixes expr-test.cc to work with codegen as it's
originally intended. Fixing it uncovers a couple of bugs fixed
in this patch:
IMPALA-4705: When an IR function is materialized, its
function body is parsed to find all its callee functions
to be materialized too. However, the old code doesn't
detect callee fnctions referenced indirectly (e.g. a
callee function passed as argument to another function).
This change fixes the problem above inspecting the use
lists of llvm::Function objects. When parsing the bitcode
module into memory, LLVM already establishes a use list
for each llvm::Value object which llvm::Function is a
subclass of. A use list contains all the locations in
the module in which the Value is referenced. For a
llvm::Function object, that would be its call sites and
constant expressions referencing the functions. By using
the use lists of llvm::Function in the module, a global
map is established at Impala initialization time to map
functions to their corresponding callee functions. This
map is then used when materializing a function to ensure
all its callee functions are also materialized recursively.
IMPALA-4779: conditional function isfalse(), istrue(),
isnotfalse(), isnotrue() aren't cross-compiled so they
will lead to unexpected query failure when codegen is enabled.
This change will cross-compile these functions.
IMPALA-4780: next_day() always returns NULL when codegen
is enabled. The bound checks for next_day() use some class
static variables initialized in the global constructors
(@llvm.global_ctors). However, we never execute the global
constructors before calling the JIT compiled functions.
This causes these variables to remain as zero, causing all
executions of next_day() to fail the bound checks. The reason
why these class static variables aren't compiled as global
constants in LLVM IR is that TimestampFunctions::MIN_YEAR is
not a compile time constant. This change fixes the problem
above by setting TimestampFunctions::MIN_YEAR to a known constant
value. A DCHECK is added to verify that it matches the value
defined in the boost library.
Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timestamp-functions.h
M be/src/runtime/exec-env.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/service/fe-support.cc
M be/src/service/fe-support.h
M be/src/testutil/test-udfs.cc
M testdata/workloads/functional-query/queries/QueryTest/udf.test
M tests/query_test/test_udfs.py
18 files changed, 224 insertions(+), 232 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/5732/3
--
To view, visit http://gerrit.cloudera.org:8080/5732
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 2:
(4 comments)
Looking better, just some minor things
http://gerrit.cloudera.org:8080/#/c/5732/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 194: if (IsDefinedInImpalad(fn_name)) continue;
I still don't entirely understand this. I understand why we don't want to materialize if it's defined in the Impala daemon, but I don't understand why we include gv -> fn mappings if the function isn't defined in the impala daemon, but not if the function is defined in the impala daemon. I don't see anything in the comment for 'callee_map_' that explains this either.
http://gerrit.cloudera.org:8080/#/c/5732/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 63: DECLARE_bool(fe_support_disable_codegen);
I don't think we want to expose this as a command-line option, do we?
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
> Without defining MIN_YEAR as a compilation constant, there isn't much to be
I agree it probably doesn't matter too much, but we should generally just put the constants in header files so that we're consistent with the pattern.
http://gerrit.cloudera.org:8080/#/c/5732/2/be/src/exprs/timestamp-functions.h
File be/src/exprs/timestamp-functions.h:
Line 42: static const int64_t MAX_YEAR_INTERVAL;
I think we should define the constant values here, so that they can be inlined in any source files
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5732/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 194: if (IsDefinedInImpalad(fn_name)) continue;
> And to elaborate a bit more, we choose to materialize a callee function ref
Sounds reasonable - I think we just need some comments explaining it
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5732/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 194: if (IsDefinedInImpalad(fn_name)) continue;
> The intention is for the 'callee_map_' to only have functions which need to
And to elaborate a bit more, we choose to materialize a callee function referenced by another function so they can be inlined for further optimization. This is not the cae for functions referenced by global variables only.
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#4).
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
This change fixes expr-test.cc to work with codegen as it's
originally intended. Fixing it uncovers a couple of bugs fixed
in this patch:
IMPALA-4705: When an IR function is materialized, its
function body is parsed to find all its callee functions
to be materialized too. However, the old code doesn't
detect callee fnctions referenced indirectly (e.g. a
callee function passed as argument to another function).
This change fixes the problem above inspecting the use
lists of llvm::Function objects. When parsing the bitcode
module into memory, LLVM already establishes a use list
for each llvm::Value object which llvm::Function is a
subclass of. A use list contains all the locations in
the module in which the Value is referenced. For a
llvm::Function object, that would be its call sites and
constant expressions referencing the functions. By using
the use lists of llvm::Function in the module, a global
map is established at Impala initialization time to map
functions to their corresponding callee functions. This
map is then used when materializing a function to ensure
all its callee functions are also materialized recursively.
IMPALA-4779: conditional function isfalse(), istrue(),
isnotfalse(), isnotrue() aren't cross-compiled so they
will lead to unexpected query failure when codegen is enabled.
This change will cross-compile these functions.
IMPALA-4780: next_day() always returns NULL when codegen
is enabled. The bound checks for next_day() use some class
static variables initialized in the global constructors
(@llvm.global_ctors). However, we never execute the global
constructors before calling the JIT compiled functions.
This causes these variables to remain as zero, causing all
executions of next_day() to fail the bound checks. The reason
why these class static variables aren't compiled as global
constants in LLVM IR is that TimestampFunctions::MIN_YEAR is
not a compile time constant. This change fixes the problem
above by setting TimestampFunctions::MIN_YEAR to a known constant
value. A DCHECK is added to verify that it matches the value
defined in the boost library.
Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timestamp-functions.h
M be/src/runtime/exec-env.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/service/fe-support.cc
M be/src/service/fe-support.h
M be/src/testutil/test-udfs.cc
M testdata/workloads/functional-query/queries/QueryTest/udf.test
M tests/query_test/test_udfs.py
18 files changed, 225 insertions(+), 232 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/5732/4
--
To view, visit http://gerrit.cloudera.org:8080/5732
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 4:
> Quite a bit longer. 5~6 mins in debug builds on my machine. I think
> it's worth it given that it covers pretty much all the built-in
> functions.
yeah, I'm guessing this was originally disabled due to the long execution time. but i agree we need to get this coverage somehow. are there anythings we can do (maybe in the future) to speed this up?
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/5732/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
PS4, Line 326: n both modules. For the latter case, it's
> They are not necessarily just function declarations in the new module. They
When you say "to be safe, we need to call MaterializeFunctionHelper with the maps of the main module and the new module", where do we create the maps of the new module?
The new comment is clearer, thanks. But I'm still confused as to why it's okay to not materialize the function from the new module.
http://gerrit.cloudera.org:8080/#/c/5732/5/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 173: RETURN_IF_ERROR(init_codegen->MaterializeModule());
seems fine, but why is this needed now but not before? oh i guess it's required to get the uses list used by FindGlobalUsers()?
PS5, Line 185: functions
functions / global-variables
(unless you change the code due to the next comment)
Line 201: fns_to_always_materialize_.insert(fn_name);
if we put it in fns_to_always_materialize_ (and therefore will be materialized), why does it also go into fn_refs_map_? i.e. should we simplify fn_refs_map_ to just be map from function to functions? doesn't seem like we should care about references from global vars when traversing the call graph. (And then the name CalleeMap would have made sense but new name is fine too, or CallGraph which is what it would be, right?).
PS5, Line 328: Note that
: // linking will cause functions defined only in the new module to be materialized.
does linkModule() actually materialize functions? or is this saying that once the new module is linked in, the new definitions will be chosen when doing module_->materialize(fn)? or something else?
http://gerrit.cloudera.org:8080/#/c/5732/5/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
PS5, Line 146: be
by
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/5732/5/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 173: RETURN_IF_ERROR(init_codegen->MaterializeModule());
> seems fine, but why is this needed now but not before? oh i guess it's requ
Yup. We cannot find uses without materializing the module first. That's why we do it once at init time.
PS5, Line 185: functions
> functions / global-variables
Took the suggestion in the next comment.
Line 201: fns_to_always_materialize_.insert(fn_name);
> if we put it in fns_to_always_materialize_ (and therefore will be materiali
Done
PS5, Line 328: Note that
: // linking will cause functions defined only in the new module to be materialized.
> does linkModule() actually materialize functions? or is this saying that on
linkModule() will materialize function.
http://gerrit.cloudera.org:8080/#/c/5732/5/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
PS5, Line 146: be
> by
Done
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
This change fixes expr-test.cc to work with codegen as it's
originally intended. Fixing it uncovers a couple of bugs fixed
in this patch:
IMPALA-4705: When an IR function is materialized, its
function body is parsed to find all its callee functions
to be materialized too. However, the old code doesn't
detect callee fnctions referenced indirectly (e.g. a
callee function passed as argument to another function).
This change fixes the problem above inspecting the use
lists of llvm::Function objects. When parsing the bitcode
module into memory, LLVM already establishes a use list
for each llvm::Value object which llvm::Function is a
subclass of. A use list contains all the locations in
the module in which the Value is referenced. For a
llvm::Function object, that would be its call sites and
constant expressions referencing the functions. By using
the use lists of llvm::Function in the module, a global
map is established at Impala initialization time to map
functions to their corresponding callee functions. This
map is then used when materializing a function to ensure
all its callee functions are also materialized recursively.
IMPALA-4779: conditional function isfalse(), istrue(),
isnotfalse(), isnotrue() aren't cross-compiled so they
will lead to unexpected query failure when codegen is enabled.
This change will cross-compile these functions.
IMPALA-4780: next_day() always returns NULL when codegen
is enabled. The bound checks for next_day() use some class
static variables initialized in the global constructors
(@llvm.global_ctors). However, we never execute the global
constructors before calling the JIT compiled functions.
This causes these variables to remain as zero, causing all
executions of next_day() to fail the bound checks. The reason
why these class static variables aren't compiled as global
constants in LLVM IR is that TimestampFunctions::MIN_YEAR is
not a compile time constant. This change fixes the problem
above by setting TimestampFunctions::MIN_YEAR to a known constant
value. A DCHECK is added to verify that it matches the value
defined in the boost library.
Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
Reviewed-on: http://gerrit.cloudera.org:8080/5732
Reviewed-by: Michael Ho <kw...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timestamp-functions.h
M be/src/service/fe-support.cc
M be/src/service/fe-support.h
M be/src/service/impalad-main.cc
M be/src/testutil/test-udfs.cc
M testdata/workloads/functional-query/queries/QueryTest/udf.test
M tests/query_test/test_udfs.py
16 files changed, 225 insertions(+), 234 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Michael Ho: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/5732
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5732
to look at the new patch set (#6).
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
This change fixes expr-test.cc to work with codegen as it's
originally intended. Fixing it uncovers a couple of bugs fixed
in this patch:
IMPALA-4705: When an IR function is materialized, its
function body is parsed to find all its callee functions
to be materialized too. However, the old code doesn't
detect callee fnctions referenced indirectly (e.g. a
callee function passed as argument to another function).
This change fixes the problem above inspecting the use
lists of llvm::Function objects. When parsing the bitcode
module into memory, LLVM already establishes a use list
for each llvm::Value object which llvm::Function is a
subclass of. A use list contains all the locations in
the module in which the Value is referenced. For a
llvm::Function object, that would be its call sites and
constant expressions referencing the functions. By using
the use lists of llvm::Function in the module, a global
map is established at Impala initialization time to map
functions to their corresponding callee functions. This
map is then used when materializing a function to ensure
all its callee functions are also materialized recursively.
IMPALA-4779: conditional function isfalse(), istrue(),
isnotfalse(), isnotrue() aren't cross-compiled so they
will lead to unexpected query failure when codegen is enabled.
This change will cross-compile these functions.
IMPALA-4780: next_day() always returns NULL when codegen
is enabled. The bound checks for next_day() use some class
static variables initialized in the global constructors
(@llvm.global_ctors). However, we never execute the global
constructors before calling the JIT compiled functions.
This causes these variables to remain as zero, causing all
executions of next_day() to fail the bound checks. The reason
why these class static variables aren't compiled as global
constants in LLVM IR is that TimestampFunctions::MIN_YEAR is
not a compile time constant. This change fixes the problem
above by setting TimestampFunctions::MIN_YEAR to a known constant
value. A DCHECK is added to verify that it matches the value
defined in the boost library.
Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timestamp-functions.h
M be/src/service/fe-support.cc
M be/src/service/fe-support.h
M be/src/service/impalad-main.cc
M be/src/testutil/test-udfs.cc
M testdata/workloads/functional-query/queries/QueryTest/udf.test
M tests/query_test/test_udfs.py
16 files changed, 224 insertions(+), 234 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/5732/6
--
To view, visit http://gerrit.cloudera.org:8080/5732
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Dan Hecht, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5732
to look at the new patch set (#7).
Change subject: IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
......................................................................
IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
This change fixes expr-test.cc to work with codegen as it's
originally intended. Fixing it uncovers a couple of bugs fixed
in this patch:
IMPALA-4705: When an IR function is materialized, its
function body is parsed to find all its callee functions
to be materialized too. However, the old code doesn't
detect callee fnctions referenced indirectly (e.g. a
callee function passed as argument to another function).
This change fixes the problem above inspecting the use
lists of llvm::Function objects. When parsing the bitcode
module into memory, LLVM already establishes a use list
for each llvm::Value object which llvm::Function is a
subclass of. A use list contains all the locations in
the module in which the Value is referenced. For a
llvm::Function object, that would be its call sites and
constant expressions referencing the functions. By using
the use lists of llvm::Function in the module, a global
map is established at Impala initialization time to map
functions to their corresponding callee functions. This
map is then used when materializing a function to ensure
all its callee functions are also materialized recursively.
IMPALA-4779: conditional function isfalse(), istrue(),
isnotfalse(), isnotrue() aren't cross-compiled so they
will lead to unexpected query failure when codegen is enabled.
This change will cross-compile these functions.
IMPALA-4780: next_day() always returns NULL when codegen
is enabled. The bound checks for next_day() use some class
static variables initialized in the global constructors
(@llvm.global_ctors). However, we never execute the global
constructors before calling the JIT compiled functions.
This causes these variables to remain as zero, causing all
executions of next_day() to fail the bound checks. The reason
why these class static variables aren't compiled as global
constants in LLVM IR is that TimestampFunctions::MIN_YEAR is
not a compile time constant. This change fixes the problem
above by setting TimestampFunctions::MIN_YEAR to a known constant
value. A DCHECK is added to verify that it matches the value
defined in the boost library.
Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timestamp-functions.h
M be/src/service/fe-support.cc
M be/src/service/fe-support.h
M be/src/service/impalad-main.cc
M be/src/testutil/test-udfs.cc
M testdata/workloads/functional-query/queries/QueryTest/udf.test
M tests/query_test/test_udfs.py
16 files changed, 225 insertions(+), 234 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/5732/7
--
To view, visit http://gerrit.cloudera.org:8080/5732
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I40fdb035a565ae2f9c9fbf4db48a548653ef7608
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4705, IMPALA-4779, IMPALA-4780: Fix some Expr bugs with codegen
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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:
Quick question: how long does expr-test take to run now?
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No