Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/9986 )
Change subject: IMPALA-3307: Add support for IANA time-zone db ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/util/filesystem-util.h File be/src/util/filesystem-util.h: http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/util/filesystem-util.h@57 PS2, Line 57: static Status GetRealPath( Shouldn't you mentioned that this should be called on sym links? (or do I misunderstand something?) I think that what a "real path" is should need some further explanation. http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/util/filesystem-util.h@60 PS2, Line 60: Is it is nit: if it is http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/util/hdfs-util.h File be/src/util/hdfs-util.h: http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/util/hdfs-util.h@59 PS2, Line 59: /// Returns basename of 'path'. Could you add an example to the comment? http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/util/time.h File be/src/util/time.h: http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/util/time.h@123 PS2, Line 123: /// Converts input microseconds-since-epoch to date-time string in 'tz' time zone. Could you mention 'p' as well? http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/util/time.cc File be/src/util/time.cc: http://gerrit.cloudera.org:8080/#/c/9986/2/be/src/util/time.cc@167 PS2, Line 167: const char* fmt = (p == TimePrecision::Millisecond) ? fmt_millisec : For me this 3 layers of embedded ternary operators isn't that readable. What about a switch? -- To view, visit http://gerrit.cloudera.org:8080/9986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I93c1fbffe81f067919706e30db0a34d0e58e7e77 Gerrit-Change-Number: 9986 Gerrit-PatchSet: 2 Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 20 Apr 2018 14:28:51 +0000 Gerrit-HasComments: Yes
