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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
