Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9376 )
Change subject: IMPALA-6537: Add missing ODBC scalar functions ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/9376/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/9376/1/be/src/exprs/expr-test.cc@6059 PS1, Line 6059: TestStringValue("monthname(cast('2011-01-18 09:10:11.000000' as timestamp))", "January"); Let's exhaustively test quarter() somewhere here as well. We should cover all possible quarter values [1-4]. http://gerrit.cloudera.org:8080/#/c/9376/1/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/9376/1/be/src/exprs/timestamp-functions-ir.cc@211 PS1, Line 211: case 1: return StringVal(SUNDAY); I think this is doing an unnecessary strlen() as well. Probably simplest to directly pass in the length as a constant. We can probably have the compiler optimize this if we use constexpr cleverly, but the fact that I'm not 100% sure makes me think just putting the constant is easier for me and others to understand. http://gerrit.cloudera.org:8080/#/c/9376/1/be/src/exprs/timestamp-functions-ir.cc@241 PS1, Line 241: return StringVal(ts_value.date().month().as_long_string()); This can be improved: * avoid boost; that's generally a good principle, since it's hard to say what's happening in that function; I checked and it's just an array lookup, but * this StringVal c'tor runs strlen() to get the length, which is wasted work * I think it's simplest to have constant month names like we do for day names (see TimestampFunctions::MONDAY) http://gerrit.cloudera.org:8080/#/c/9376/1/be/src/exprs/udf-builtins-ir.cc File be/src/exprs/udf-builtins-ir.cc: http://gerrit.cloudera.org:8080/#/c/9376/1/be/src/exprs/udf-builtins-ir.cc@129 PS1, Line 129: if (unit == "quarter") return TExtractField::QUARTER; move between year and month to keep logical progression http://gerrit.cloudera.org:8080/#/c/9376/1/be/src/exprs/udf-builtins-ir.cc@156 PS1, Line 156: case TExtractField::QUARTER: move between year and month to keep logical progression http://gerrit.cloudera.org:8080/#/c/9376/1/be/src/exprs/udf-builtins-ir.cc@178 PS1, Line 178: return (m - 1) / 3 + 1; IntVal(<calculation>) to be more explicit http://gerrit.cloudera.org:8080/#/c/9376/1/common/thrift/Exprs.thrift File common/thrift/Exprs.thrift: http://gerrit.cloudera.org:8080/#/c/9376/1/common/thrift/Exprs.thrift@81 PS1, Line 81: QUARTER, move between year and month http://gerrit.cloudera.org:8080/#/c/9376/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/9376/1/fe/src/main/cup/sql-parser.cup@2778 PS1, Line 2778: /* Since "IF", "REPLACE", "TRUNCATE" are keywords, need to special case these functions */ Make this comment more generic like: /* Separate rules for function names that are also keywords */ -- To view, visit http://gerrit.cloudera.org:8080/9376 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia60af2b4de8c098be7ecb3e60840e459ae10d499 Gerrit-Change-Number: 9376 Gerrit-PatchSet: 1 Gerrit-Owner: Greg Rahn <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Comment-Date: Wed, 21 Feb 2018 05:59:00 +0000 Gerrit-HasComments: Yes
