Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/9283 )
Change subject: Add KuduDataTypeToColumnType default case ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/9283/1/be/src/exec/kudu-util.cc File be/src/exec/kudu-util.cc: http://gerrit.cloudera.org:8080/#/c/9283/1/be/src/exec/kudu-util.cc@196 PS1, Line 196: default: return ColumnType(PrimitiveType::INVALID_TYPE); > It didn't appear intentional given there was a "fall through" to a `return I believe the "return INVALID_TYPE" is required by at least some compilers (even though of course as you say it can never be executed). If not, we can remove it, or else leave a comment. When going the other direction, Impala type -> Kudu type, its genuinely non-exhaustive (i.e. there are types in Impala that have no corresponding type in Kudu, but all Kudu types have a corresponding Impala type), so the default makes more sense. If there are any other places where we're going Kudu type -> Impala type where a 'default' is used, I would argue we should remove those defaults. Its also a little different for the FE, because we don't have as much control over when maven starts pulling in the new Kudu client, since it happens automatically, but for the BE we do have control. -- To view, visit http://gerrit.cloudera.org:8080/9283 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f7d9e16070b66274107a2bff52cbe8f3ae9b8f1 Gerrit-Change-Number: 9283 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Grant Henke <granthe...@gmail.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Mon, 12 Feb 2018 19:32:23 +0000 Gerrit-HasComments: Yes
