impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4594: WriteSlot and CodegenWriteSlot handle escaped NULL slots differently
Date Wed, 07 Dec 2016 22:15:09 GMT
Dan Hecht has posted comments on this change.

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

Patch Set 3:

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?
File be/src/exec/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I858e427ad7c2b2da8c2bb657be06b7443655781f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message