Dan Hecht has posted comments on this change. Change subject: IMPALA-1878: Support INSERT and LOAD DATA on S3 and between filesystems ......................................................................
Patch Set 24: Code-Review+1 (4 comments) +2 for be. Python test changes look fine, if MichaelB/Henry are happy with the rest of the python, then let's consider those +2. So, just need a +2 for fe changes from Alex or Marcel. http://gerrit.cloudera.org:8080/#/c/2574/24/be/src/util/hdfs-util-test.cc File be/src/util/hdfs-util-test.cc: Line 58: EXPECT_TRUE(FilesystemsMatch("hdfs://namenode/temp_dir/temp_path", "hdfs://")); why do these match? does it matter what scheme the default_fs has? i.e. if default_fs=s3a://blah, should this still return true? what should happen in that case? http://gerrit.cloudera.org:8080/#/c/2574/24/be/src/util/hdfs-util.cc File be/src/util/hdfs-util.cc: Line 105: && fs_b_name_length > 0 isn't this implied by the previous if-conditional being false? i.e. they should be DCHECKs. Also, a couple comments would make it easier to read through: // Neither is fully qualified: both are on default_fs. if (fs_a_name_length == 0 && fs_b_name_length == 0) return true; // One is a relative path: check fully-qualified one against default_fs. if (fs_a_name_length == 0) { return strncmp(path_b, default_fs, default_fs_name_length) == 0; } if (fs_b_name_length == 0) { return strncmp(path_a, default_fs, default_fs_name_length) == 0; } // Both fully qualified: check the filesystem prefix. if (fs_a_name_length != fs_b_name_length) return false; return strncmp(path_a, path_b, fs_a_name_length) == 0; http://gerrit.cloudera.org:8080/#/c/2574/24/tests/metadata/test_ddl.py File tests/metadata/test_ddl.py: Line 70: @SkipIfS3.qualified_path you don't have to do it in this change, but can't this test be fixed? http://gerrit.cloudera.org:8080/#/c/2574/24/tests/metadata/test_hdfs_encryption.py File tests/metadata/test_hdfs_encryption.py: Line 31: qualified_path or should this be hdfs_encryption? qualified_path is really specifically for IMPALA-1872; didn't notice that being used here. -- 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: 24 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
