Michael Ho has posted comments on this change. Change subject: Reduce dependencies on inline header functions ......................................................................
Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/2485/3/be/src/exec/hdfs-text-scanner.cc File be/src/exec/hdfs-text-scanner.cc: Line 26: #include "runtime/runtime-filter.inline.h" May be I missed it but doesn't seem to see any reference to RunTimeFilter in this file ? http://gerrit.cloudera.org:8080/#/c/2485/3/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: Line 41: #include "util/bloom-filter.h" Is this actually needed ? http://gerrit.cloudera.org:8080/#/c/2485/3/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 31: #include "runtime/runtime-filter.inline.h" Is this for RegisterFilter() only ? If so, should we #include runtime-filter.h instead ? http://gerrit.cloudera.org:8080/#/c/2485/3/be/src/runtime/buffered-tuple-stream.cc File be/src/runtime/buffered-tuple-stream.cc: Line 23: runtime/string-value.inline.h" Would #include "runtime/string-value.h" be sufficient ? 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, > Quite possibly but I wanted to avoid making any changes that had perf impli Fair enough. I believe FromInt() is called from decimal-operator.cc so it should be okay to move it there but it's fine to keep it here too. 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: > We normally have a blank line between header groups. The first group is the I see. 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) { > Maybe - it's hard to know if it has zero impact since it's on some hot path Don't we only call this for interpretation ? Normally, we would use the codegen'ed version, right ? I think it's okay to preserve the inlining in this change to avoid unexpected perf regression although I am not sure if we are gaining much to inline this huge function. Line 336: RawValue::PrintValue Do you know why only this version of PrintValue() is inlined ? It seems to be used by HdfsTextTableWriter so may be that's a common use case ? http://gerrit.cloudera.org:8080/#/c/2485/3/be/src/runtime/timestamp-value.cc File be/src/runtime/timestamp-value.cc: Line 16: > I think this is following the usual way of grouping include includes since I see. -- 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
