Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8508 )

Change subject: IMPALA-5237: Support a quoted string in date/time format
......................................................................


Patch Set 10:

(1 comment)

Sorry for the slow turnaround time here - things got crazy for a while.

http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:

http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc@175
PS9, Line 175:   // e.g. '''foo''bar''' => 'foo'bar'
> Let me provide a more intuitive algorithm to find escaped single quotes bet
I think we still need a comment like my above one, since we are still treating 
the last single quote found as the start of a new string.

To be honest I think the new approach less intuitive, because we still split 
the literal token up into multiple tokens if there are escaped quotes in the 
middle of the string. It seems like the approach only changes when dealing with 
quotes at the end of the string.

E.g. my understanding is that "'foo''bar'" is still broken into two tokens, 
"foo'" and "bar", whereas "foobar'''" is handled as a single token "foo'".

I think fundamentally we need to do this token-splitting because of the memory 
management - we can't convert "'foo''bar'" into a single token 'foo'bar' 
without creating a new string and copying the data. So my preference is to use 
the previous approach and explain it clearly.



--
To view, visit http://gerrit.cloudera.org:8080/8508
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
Gerrit-Change-Number: 8508
Gerrit-PatchSet: 10
Gerrit-Owner: Kim Jin Chul <jinc...@gmail.com>
Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com>
Gerrit-Reviewer: Kim Jin Chul <jinc...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Mar 2018 01:09:53 +0000
Gerrit-HasComments: Yes

Reply via email to