Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201612072215.uB7MF9hC012380@ip-10-146-233-104.ec2.internal>
Date: Wed, 7 Dec 2016 22:15:09 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Sailesh Mukil <sailesh@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Michael Ho <kwho@cloudera.com>
Reply-To: dhecht@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4594=3A_WriteSlot_and_CodegenWriteSlot_handle_escaped_NULL_slots_differently=0A?=
In-Reply-To: <gerrit.1481011800000.I858e427ad7c2b2da8c2bb657be06b7443655781f@gerrit.cloudera.org>
References: <gerrit.1481011800000.I858e427ad7c2b2da8c2bb657be06b7443655781f@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Wed, 07 Dec 2016 22:15:15 -0000

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4594: WriteSlot and CodegenWriteSlot handle escaped NULL slots differently
......................................................................


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5377/3//COMMIT_MSG
Commit Message:

PS3, Line 7: escaped NULL
does this mean '\N', or can it be something else? if it means that, let's just say that (I wasn't sure what it meant at first).


PS3, Line 9: receives
this could use more explanation as to where these values come from and where this convention is defined (is it DelimitedTextParser?)


Line 20: cases, even though the slot is a NULL slot.
i don't understand the explanation given the code you're changing in CodegenWriteSlot used to mark anything <= 0 as NULL.

The code change itself looks fine though, it's just hard to follow this explanation.  Could you try to clarify it?


PS3, Line 22: Things worked fine
Was confused by this statement since you go on to say that invalid slots did not return an error.  Do you mean "NULL slots worked..." here?


PS3, Line 23: and NULL
what does this mean, given that you just said the code to check for NULL slots doesn't work?


http://gerrit.cloudera.org:8080/#/c/5377/3/be/src/exec/hdfs-scanner.cc
File be/src/exec/hdfs-scanner.cc:

Line 460:       // Convert length to positive if it is negative.
where do we explain what negative lengths mean? i think a reminder of what it means would be nice here. You can figure it out by reading the TODO but I think it would be good to say so directly.


PS3, Line 461: escaped strings
I think this would be clearer to say: strings with escape charaters.
Or: strings that need unescaping.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I858e427ad7c2b2da8c2bb657be06b7443655781f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes