Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/12481 )
Change subject: IMPALA-7368: Add initial support for DATE type ...................................................................... Patch Set 19: (4 comments) http://gerrit.cloudera.org:8080/#/c/12481/19//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12481/19//COMMIT_MSG@20 PS19, Line 20: Explicit casting between DATE and other types: Please mention that invalid conversion result in a warning, which is different than the other casts work. http://gerrit.cloudera.org:8080/#/c/12481/19//COMMIT_MSG@24 PS19, Line 24: truncted typo: truncated http://gerrit.cloudera.org:8080/#/c/12481/19/be/src/exprs/cast-functions-ir.cc File be/src/exprs/cast-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/12481/19/be/src/exprs/cast-functions-ir.cc@312 PS19, Line 312: if (UNLIKELY(!dv.IsValid())) { : ctx->AddWarning("String to Date parse failed."); : return DateVal::null(); : } I totally prefer returning an error if we are casting a constant, but I am not sure about values fetched during the query. What will happen is that warnings will be added to the list of general errors, and when the number of errors reach MAX_ERRORS (query option, 100 by default in my minicluster), the query will fail. If we want to fail more consistently, SetError() could be called instead of AddWarning(). Postgres seems to do this, it fails at the first row it cannot cast. On the other side, the "return NULL without warning" seems like a useful thing when the values are from a table, as it makes it easy for the user to handle the case when not every string is a valid date. So I would prefer to return error only for constant casts. FunctionContext has a IsArgConstant() functions, but I don't know whether it works during constant folding. If we want to postpone this decision, an error could be returned for now, and we could change Impala to be more forgiving in the future and have some special logic for constant casts. http://gerrit.cloudera.org:8080/#/c/12481/19/be/src/exprs/cast-functions-ir.cc@323 PS19, Line 323: ctx->AddWarning("Timestamp to Date conversion failed. " : "Timestamp has no date component."); Note that "dateless timestamps" are nearly useless, see IMPALA-5942. I regret not dropping them among the breaking changes in Impala 3.0. A comment could be added to related functions/tests to avoid giving the impression that these can be actually used for things. -- 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: 19 Gerrit-Owner: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 10 Apr 2019 14:35:49 +0000 Gerrit-HasComments: Yes
