Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11919 )
Change subject: IMPALA-5031: signed overflow in TimestampValue ...................................................................... Patch Set 3: > > I like the current solution, but I think that it should break > > several tests in timestamp-test.cc, for example > > https://github.com/apache/impala/blob/ecf12bec42e11262b88dc0993e375fe4d8acaafb/be/src/runtime/timestamp-test.cc#L290 > > Earlier, you said, "I would prefer to avoid the overflow by > changing the interface to handle nanos only in the -999'999'999 .. > + 999'999'999 range, and treat other values as invalid timestamps." > > I gather from this that you would prefer to alter the test to meet > this restriction than you would to widen the allowed values in > FromUnixTimeNanos? As another comment: if changing the semantics has subtleties, I'd MUCH rather keep the semantics the same, altering only the path where there is undefined behavior, even for a marginal slowdown. -- To view, visit http://gerrit.cloudera.org:8080/11919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaad158e6634314a5690a43a0cc04426c1aba8f41 Gerrit-Change-Number: 11919 Gerrit-PatchSet: 3 Gerrit-Owner: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Comment-Date: Mon, 11 Mar 2019 22:42:34 +0000 Gerrit-HasComments: No
