Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12481 )
Change subject: IMPALA-7368: Add initial support for DATE type ...................................................................... Patch Set 6: (10 comments) I did a pass over the backend code. I think it's looking pretty good. Most comments I had were actually about existing issues in the codebase. http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/exprs/aggregate-functions-ir.cc@386 PS6, Line 386: if (UNLIKELY(src.is_null)) return DateVal::null(); It's weird to handle is_null here and not in GetValue(). I think it's unnecessary. Seems to be carried over from the other functions. I'm ok if you don't want to handle it right now, but maybe file a JIRA if you agree with me that it's wrong? http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/exprs/cast-functions-ir.cc File be/src/exprs/cast-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/exprs/cast-functions-ir.cc@179 PS6, Line 179: lexical_cast It would be better to use ToString() directly rather than involving the indirection of lexical_cast and operator<<. http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/exprs/conditional-functions-ir.cc File be/src/exprs/conditional-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/exprs/conditional-functions-ir.cc@35 PS6, Line 35: IS_NULL_COMPUTE_FUNCTION(BooleanVal); I know we'd talked about how to make it easier to add more data types in future. This is one case where we could have some macros to stamp out functions like this for each *Val type. Not saying we need to do it here, just thinking. http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/exprs/expr-test.cc@3466 PS6, Line 3466: a are http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/exprs/slot-ref.cc File be/src/exprs/slot-ref.cc: http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/exprs/slot-ref.cc@451 PS6, Line 451: TimestampVal SlotRef::GetTimestampVal( These functions look regular enough that we could maybe stamp them out automatically from all types with a macro, except for the parameterised types like DecimalVal. Don't need to fix now, just thinking. http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/runtime/date-value.h File be/src/runtime/date-value.h: http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/runtime/date-value.h@96 PS6, Line 96: underlaying underlying http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/runtime/date-value.cc File be/src/runtime/date-value.cc: http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/runtime/date-value.cc@68 PS6, Line 68: (void)DateParser::Parse(str, len, &dv); I think you'll need to use discard_result() here. I can't remember exactly but either GCC7 or Clang doesn't consider this to be actually discarding the return value. http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/service/fe-support.cc File be/src/service/fe-support.cc: http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/service/fe-support.cc@639 PS6, Line 639: std::s nit: std:: prefix not needed since it's imported into the global namespace by common/names.h http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/service/hs2-util.cc File be/src/service/hs2-util.cc: http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/service/hs2-util.cc@539 PS6, Line 539: RawValue::PrintValue(value, TYPE_DATE, -1, &(hs2_col_val->stringVal.value)); Can we just use StringValue::ToString() instead? Seems like a lot of indirection to use this. Oh I guess TIMESTAMP does this below, oh well. http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/util/symbols-util.cc File be/src/util/symbols-util.cc: http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/util/symbols-util.cc@135 PS6, Line 135: case TYPE_DATE: Another example where it's just stamped out for each *Val type. -- To view, visit http://gerrit.cloudera.org:8080/12481 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea8155ef09557e0afa2f8b2d0b2dc9d0896dc30f Gerrit-Change-Number: 12481 Gerrit-PatchSet: 6 Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 26 Feb 2019 01:58:29 +0000 Gerrit-HasComments: Yes
