Dan Hecht has posted comments on this change.
Change subject: IMPALA-3567: move ExecOption profile helpers to RuntimeProfile
......................................................................
Patch Set 4: Code-Review+2
(5 comments)
I still think it'd be clearer to clean up how codegen reporting happens so we
just have one mechanism (Status) for passing this info rather than various
subsets of {bool, status, string}, but you don't have to do that cleanup in
this commit.
http://gerrit.cloudera.org:8080/#/c/4188/3/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
PS3, Line 465: ||
> Yes, that was clang-format. It looks like there was a deliberate decision t
Okay, if that was the agreed on format moving forward, fine with me. (This new
format happens to be my preference, but was inconsistent with our usual
formatting).
http://gerrit.cloudera.org:8080/#/c/4188/3/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:
PS3, Line 207: Msg(bo
> Did the rename.
It still seems nice to simplify this and unify these cases in the future. i.e.
codegen status in this case would be "Disabled due to disable_codegen=true
query option" or whatever. But this can be done later.
PS3, Line 211: OK
> They're partially independent. codegen_enabled implies status is ok, but vi
See above -- seems best to unify how this info all gets passed along, but you
don't have to do that in this commit.
http://gerrit.cloudera.org:8080/#/c/4188/4/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:
Line 212: /// 'codegen_enabled' is true (e.g. if codegen is disabled by a
query option,
not?
PS4, Line 214: Msg
Oh, I didn't mean you had to rename this one, since this one does take a
Status, though I suppose since it doesn't use the status directly to decide,
this new name is also okay.
--
To view, visit http://gerrit.cloudera.org:8080/4188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I21c1dda8f8a1d92172bf59fbc1070a6834e61913
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <[email protected]>
Gerrit-Reviewer: Alex Behm <[email protected]>
Gerrit-Reviewer: Dan Hecht <[email protected]>
Gerrit-Reviewer: Marcel Kornacker <[email protected]>
Gerrit-Reviewer: Matthew Jacobs <[email protected]>
Gerrit-Reviewer: Tim Armstrong <[email protected]>
Gerrit-HasComments: Yes
