Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14291 )
Change subject: IMPALA-8704: ISO:SQL:2016 datetime patterns - Milestone 2 ...................................................................... Patch Set 14: Code-Review+1 (3 comments) One more round. I'll +2 it afterwards. http://gerrit.cloudera.org:8080/#/c/14291/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14291/14//COMMIT_MSG@28 PS14, Line 28: This is the default behaviour What does "default behavior" mean in this context? Is it when no FX or FM modifier is used? Or FX is used but FM isn't? http://gerrit.cloudera.org:8080/#/c/14291/14/be/src/runtime/datetime-iso-sql-format-tokenizer.cc File be/src/runtime/datetime-iso-sql-format-tokenizer.cc: http://gerrit.cloudera.org:8080/#/c/14291/14/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@115 PS14, Line 115: if (fm_modifier_active_) nit: not necessary, you can just set the flag to false no matter what. http://gerrit.cloudera.org:8080/#/c/14291/14/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@237 PS14, Line 237: DCHECK(*current_pos == dt_ctx_->fmt); It would be more straightforward if ProcessFXModifier() did not take any parameters and returned a char* instead of void. No need to pass current_pos as a parameter if *current_pos is expected to be set to dt_ctx_->fmt. -- To view, visit http://gerrit.cloudera.org:8080/14291 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30d2f6656054371476aaa8bd0d51f572b9369855 Gerrit-Change-Number: 14291 Gerrit-PatchSet: 14 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Mon, 04 Nov 2019 17:12:07 +0000 Gerrit-HasComments: Yes
