[Impala-ASF-CR] IMPALA-8704: ISO:SQL:2016 datetime patterns - Milestone 2

["Attila Jeges (Code Review)" <ge...@cloudera.org>]

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 <ga...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Nov 2019 17:12:07 +0000
Gerrit-HasComments: Yes