Tim Armstrong has posted comments on this change. Change subject: Reduce dependencies on inline header functions ......................................................................
Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/2485/3/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: Line 31: inline DecimalValue<T> DecimalValue<T>::FromDouble(int precision, int scale, double d, > I wonder if all of these need to be in the header files. Can some of them b Quite possibly but I wanted to avoid making any changes that had perf implications in this patch. It's also a more involved moving template functions to .cc. files since you have to explicitly instantiate them. Line 307: } > nit: missing blank lines. Same below. Done http://gerrit.cloudera.org:8080/#/c/2485/3/be/src/runtime/raw-value-ir.cc File be/src/runtime/raw-value-ir.cc: Line 16: > nit: extra blank line. We normally have a blank line between header groups. The first group is the header for the .cc file, then the next group is other impala headers. http://gerrit.cloudera.org:8080/#/c/2485/3/be/src/runtime/raw-value.inline.h File be/src/runtime/raw-value.inline.h: Line 32: inline bool RawValue::Eq(const void* v1, const void* v2, const ColumnType& type) { > This looks pretty big. Should we inline it ? Same question for some functio Maybe - it's hard to know if it has zero impact since it's on some hot paths. I'd prefer to avoid any perf-impacting changes in this patch http://gerrit.cloudera.org:8080/#/c/2485/3/be/src/runtime/timestamp-value.cc File be/src/runtime/timestamp-value.cc: Line 16: > nit: extra blank line ? I think this is following the usual way of grouping include includes since we normally have common/names.h last in its own group. -- To view, visit http://gerrit.cloudera.org:8080/2485 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic7a2f388cd14a4427c43af2724340a2ffe8fae3d Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
