Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14705 )
Change subject: IMPALA-8800: Added support of Kudu DATE type to Impala ...................................................................... Patch Set 18: (3 comments) http://gerrit.cloudera.org:8080/#/c/14705/13/be/src/exec/kudu-util.cc File be/src/exec/kudu-util.cc: http://gerrit.cloudera.org:8080/#/c/14705/13/be/src/exec/kudu-util.cc@133 PS13, Line 133: // If the value was invalid the slot should've been null. > OK, so be it: it's just a nit. 'slot' is an Impala concept used for query planning and execution, basically it corresponds to a single instance of a column value. So this is saying that if you would've generated an invalid DATE value to insert into Kudu for a particular column, by the time we reach the point where we're inserting into Kudu it'll have been replaced by nullptr and we won't call into this function in the first place (agree its not a very clear comment, but I think its fine to leave as is). I don't think you have any tests that exercise this path, though, as all of your tests only insert date literals, and an invalid date literal will be caught in the FE by analysis. It would be good to add a test that does some date arithmetic that produces an invalid date, eg 'ADD_MONTHS(DATE '0001-01-01', -1)' and show that the inserted value ends up NULL. http://gerrit.cloudera.org:8080/#/c/14705/13/be/src/exec/kudu-util.cc@135 PS13, Line 135: Invalid DateValue" > OK, preserving the uniformity might be an option here. Yeah, this line should (theoretically) never be hit anyways, see the above DCHECK, so fine to leave as is. http://gerrit.cloudera.org:8080/#/c/14705/18/be/src/exec/kudu-util.cc File be/src/exec/kudu-util.cc: http://gerrit.cloudera.org:8080/#/c/14705/18/be/src/exec/kudu-util.cc@194 PS18, Line 194: int32_t days = reinterpret_cast<const DateValue*>(value)->Value(); Please use ConvertDateValue here -- To view, visit http://gerrit.cloudera.org:8080/14705 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91656749a58ac769b54c2a63bdd4f85c89520b32 Gerrit-Change-Number: 14705 Gerrit-PatchSet: 18 Gerrit-Owner: Volodymyr Verovkin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Tue, 10 Mar 2020 20:12:52 +0000 Gerrit-HasComments: Yes
