Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11919 )
Change subject: IMPALA-5031: signed overflow in TimestampValue ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11919/1/be/src/runtime/timestamp-value.inline.h File be/src/runtime/timestamp-value.inline.h: http://gerrit.cloudera.org:8080/#/c/11919/1/be/src/runtime/timestamp-value.inline.h@66 PS1, Line 66: int64_t nanos > A couple of clarifying questions: "When you say "all (non-test) callers pass int32/uint32", so you mean as the value of nanos or as the value of unix time?" I meant the value of nanos. "When you say "overflow of unix_time is only possible near the min/max value representable on 64 bits", are the "64 bits" you're referring to the ones in unix_time?" Yes, I meant unix_time. "When you say "changing the interface to handle nanos only in the -999'999'999 .. + 999'999'999 range", you mean adding a comment and a DCHECK and changing callers if necessary, or do you mean something else about the interface?" I prefer the comment + adding the validation to this function. Some of the callers are scanners, so if the data in the file is corrupt, then it is possible to get out of range values here. So if we would add a DCHECK, then some files could crash a debug build. "When you say "This would mean that unix_time would be affected only in the negative case", can you explain why?" For nanos in 0..999'999'999 range SplitTime<NANOS_PER_SEC> will return 0, so unix_time time will not be affected, and nanos can be simply added in line 74 of the original code. -- 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: 1 Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Comment-Date: Mon, 11 Feb 2019 16:44:02 +0000 Gerrit-HasComments: Yes
