Henry Robinson has posted comments on this change. Change subject: IMPALA-1878: Support INSERT and LOAD DATA on S3 and between filesystems ......................................................................
Patch Set 11: (7 comments) Looks pretty good to me! I think we should let the default FS patch get in first, and then do any follow-on work required for that here. Does that sound ok with you? http://gerrit.cloudera.org:8080/#/c/2574/11/testdata/workloads/functional-query/queries/QueryTest/insert_permutation.test File testdata/workloads/functional-query/queries/QueryTest/insert_permutation.test: Line 4: '$FILESYSTEM_PREFIX/test-warehouse/insert_premutation_test' typo in the location. Presumably this requirement to specify the location goes away if S3 is the default FS? http://gerrit.cloudera.org:8080/#/c/2574/11/testdata/workloads/functional-query/queries/QueryTest/multiple-filesystems.test File testdata/workloads/functional-query/queries/QueryTest/multiple-filesystems.test: Line 33 Are we losing coverage by not having a test that tries to INSERT into a non-insertable filesystem? http://gerrit.cloudera.org:8080/#/c/2574/11/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: Line 120: # Filesystem client acts as a generic client to supported filesystems. Comment is a bit redundant. http://gerrit.cloudera.org:8080/#/c/2574/11/tests/metadata/test_ddl.py File tests/metadata/test_ddl.py: Line 70: @SkipIfS3.qualified_path FWIW, I just filed https://issues.cloudera.org/browse/IMPALA-3313 to deal with having to add multiple decorators to each test to manually select them in or out for different filesystems. http://gerrit.cloudera.org:8080/#/c/2574/11/tests/query_test/test_cancellation.py File tests/query_test/test_cancellation.py: Line 160: @SkipIfS3.jira What's the JIRA? http://gerrit.cloudera.org:8080/#/c/2574/11/tests/util/filesystem_base.py File tests/util/filesystem_base.py: Line 1: Remove blank line http://gerrit.cloudera.org:8080/#/c/2574/11/tests/util/filesystem_utils.py File tests/util/filesystem_utils.py: Line 32: speicfic specific -- 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: 11 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
