impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-1878: Support INSERT and LOAD DATA on S3 and between filesystems
Date Thu, 28 Apr 2016 04:52:26 GMT
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-1878: Support INSERT and LOAD DATA on S3 and between filesystems

Patch Set 22:

File be/src/exec/

Line 301:     // the file is not accessible until it's closed.
> I guess we should clarify this comment further because of the fact that eve

Line 312: 
> nit: remove blank line - this is all one logical block of code.
File be/src/exec/hdfs-table-sink.h:

Line 93: get_block_size
> actually, since this is a struct, we can just access block_size. We're alre
File be/src/util/

Line 60:                                 "temp_dir/temp_path"));
> let's add tests for the file scheme case. see other comments for why that's
File be/src/util/

Line 81: getNamenodeLength
> static int GetNamenodeLength ... and add a quick function header comment he
You're right. I can see how namenode could sound confusing. I changed it to GetFilesystemNameLength()
and added a comment above the function.
Please let me know if you think the name should be changed.

Line 86: skip_delimiter
> would it make sense to call this delimiter_len?  actually, since you use po
That makes sense. I got rid of the delimiter_len and added a special case for "file:".

Line 91:   if (after_authority == NULL) return strlen(path);
> I think for file scheme, we shouldn't treat the next thing as authority, ri
Yes that's right. I missed that. I've added a case for that.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I94e15ad67752dce21c9b7c1dced6e114905a942d
Gerrit-PatchSet: 22
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message