Sailesh Mukil has posted comments on this change. Change subject: IMPALA-1878: Support INSERT and LOAD DATA on S3 and between filesystems ......................................................................
Patch Set 24: (4 comments) > (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 This case is special, because for some reason if we get a path with only a scheme, then we do not have enough information to parse it. We ideally shouldn't get such a path, but we've used that as a fallback here if we don't get a defaultFS from the core-site (this should never happen though): https://github.com/cloudera/Impala/blob/cdh5-trunk/be/src/runtime/exec-env.cc#L419 For this test case, it wouldn't matter what scheme the default_fs has, because both these paths have their own schemes. So only those schemes will be taken into consideration for matching. 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 sh Yes, you're right. I've used your format and I've added DCHECKs too. 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? I'll read up on the semantics of PURGE and try to fix it for S3 or write an S3 specific PURGE test as a part of another JIRA. Filed IMPALA-3459. The only difference I know for now is that S3 doesn't have a ".Trash" folder or something equivalent, so a DROP TABLE PURGE shouldn't be that different from a regular DROP TABLE for S3. 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 f Yes hdfs_encryption is the actual reason. I've added a SkipIfS3.hdfs_encryption and used that 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
