Adam Tamas has posted comments on this change. ( http://gerrit.cloudera.org:8080/15866 )
Change subject: IMPALA-9531: Dropped support for dateless timestamps ...................................................................... Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/15866/4/be/src/exprs/scalar-expr-evaluator.cc File be/src/exprs/scalar-expr-evaluator.cc: http://gerrit.cloudera.org:8080/#/c/15866/4/be/src/exprs/scalar-expr-evaluator.cc@444 PS4, Line 444: TimestampFunctions::FromUnixPrepare(nullptr, FunctionContext::FRAGMENT_LOCAL); > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/15866/3/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/15866/3/be/src/exprs/timestamp-functions-ir.cc@167 PS3, Line 167: if (!context->IsArgConstant(1)) { > Did you find out what this check is for? Don't you break anything with simp Done http://gerrit.cloudera.org:8080/#/c/15866/4/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/15866/4/be/src/exprs/timestamp-functions-ir.cc@168 PS4, Line 168: dt_ctx->Reset(reinterpret_cast<const char*>(fmt.ptr), fmt.len) > Anyway, if you tokenize the format here then you do the tokenization for ea Done http://gerrit.cloudera.org:8080/#/c/15866/4/be/src/exprs/timestamp-functions.h File be/src/exprs/timestamp-functions.h: http://gerrit.cloudera.org:8080/#/c/15866/4/be/src/exprs/timestamp-functions.h@104 PS4, Line 104: > Please use datetime_parse_util::CastDirection We can't reach datetime_parse_util::CastDirection from here that is why I used bool. I spoke with Csaba Ringhofer about it and in the end we come to the conclusion that it is not necessary to be declared here, it is enough in the .cc because it is just there to avoid code duplication. http://gerrit.cloudera.org:8080/#/c/15866/4/be/src/exprs/timestamp-functions.cc File be/src/exprs/timestamp-functions.cc: http://gerrit.cloudera.org:8080/#/c/15866/4/be/src/exprs/timestamp-functions.cc@155 PS4, Line 155: CastDirection cast > Why don't you use the parameter type that could be directly passed to Token Done http://gerrit.cloudera.org:8080/#/c/15866/4/be/src/exprs/timestamp-functions.cc@167 PS4, Line 167: if (!parse_result) { : delete dt_ctx; : ReportBadFormat(context, datetime_parse_util::GENERAL_ERROR, fmt_val, true); : return; > Please check how to format if-else in Impala Done http://gerrit.cloudera.org:8080/#/c/15866/4/be/src/runtime/datetime-simple-date-format-parser.h File be/src/runtime/datetime-simple-date-format-parser.h: http://gerrit.cloudera.org:8080/#/c/15866/4/be/src/runtime/datetime-simple-date-format-parser.h@88 PS4, Line 88: > I wouldn't give this a default value unless you have any specific reason ma Done http://gerrit.cloudera.org:8080/#/c/15866/4/testdata/data/README File testdata/data/README: http://gerrit.cloudera.org:8080/#/c/15866/4/testdata/data/README@503 PS4, Line 503: dateless_timestamps.pa > The file name is not that verbose. After reading it it doesn't give any hin Done http://gerrit.cloudera.org:8080/#/c/15866/4/testdata/data/timestamp_text_test.txt File testdata/data/timestamp_text_test.txt: http://gerrit.cloudera.org:8080/#/c/15866/4/testdata/data/timestamp_text_test.txt@1 PS4, Line 1: > Same comment for the file name as for the parquet file: "dateless_timestamp Done http://gerrit.cloudera.org:8080/#/c/15866/4/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/15866/4/tests/query_test/test_scanners.py@391 PS4, Line 391: > flake8: E501 line too long (98 > 90 characters) Done -- To view, visit http://gerrit.cloudera.org:8080/15866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48c49bf027cc4b917849b3d58518facba372b322 Gerrit-Change-Number: 15866 Gerrit-PatchSet: 5 Gerrit-Owner: Adam Tamas <ta...@cloudera.com> Gerrit-Reviewer: Adam Tamas <ta...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Sun, 21 Jun 2020 10:58:49 +0000 Gerrit-HasComments: Yes
