impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
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:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/2574/22/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

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
Done


Line 312: 
> nit: remove blank line - this is all one logical block of code.
Done


http://gerrit.cloudera.org:8080/#/c/2574/22/be/src/exec/hdfs-table-sink.h
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
Done


http://gerrit.cloudera.org:8080/#/c/2574/22/be/src/util/hdfs-util-test.cc
File be/src/util/hdfs-util-test.cc:

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


http://gerrit.cloudera.org:8080/#/c/2574/22/be/src/util/hdfs-util.cc
File be/src/util/hdfs-util.cc:

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 http://gerrit.cloudera.org:8080/2574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94e15ad67752dce21c9b7c1dced6e114905a942d
Gerrit-PatchSet: 22
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message