Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201608292122.u7TLMqID004108@ip-10-146-233-104.ec2.internal>
Date: Mon, 29 Aug 2016 21:22:52 +0000
From: "Michael Brown (Code Review)" <gerrit@cloudera.org>
To: Alex Behm <alex.behm@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
Reply-To: mikeb@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3491=3A_Use_unique_database_fixture_in_test_ddl=2Epy=2E=0A?=
In-Reply-To: <gerrit.1472486105000.Idf667dd5e960768879c019e2037cf48ad4e4241b@gerrit.cloudera.org>
References: <gerrit.1472486105000.Idf667dd5e960768879c019e2037cf48ad4e4241b@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Mon, 29 Aug 2016 21:22:58 -0000

Michael Brown has posted comments on this change.

Change subject: IMPALA-3491: Use unique database fixture in test_ddl.py.
......................................................................


Patch Set 1:

(5 comments)

Eyes began to glaze over at last 10% of test_ddl.py. Here's some other comments for now.

http://gerrit.cloudera.org:8080/#/c/4155/1/testdata/workloads/functional-query/queries/QueryTest/alter-table.test
File testdata/workloads/functional-query/queries/QueryTest/alter-table.test:

PS1, Line 263: create external table t_part (i int) partitioned by (j int, s string)
             : location '$FILESYSTEM_PREFIX/test-warehouse/$DATABASE.db/t_part_tmp';
             : alter table t_part add partition (j=cast(2-1 as int), s='2012');
             : alter table t_part add if not exists partition (j=1, s='2012');
             : alter table t_part add if not exists partition (j=1, s='2012/withslash');
             : alter table t_part add partition (j=1, s=substring('foo2013bar', 4, 8));
Will the test framework adequately fail the test if one of these reports an error? It matters here, because unlike in some cases below, there's no SELECT at the end of this to implicitly verify they all worked.


http://gerrit.cloudera.org:8080/#/c/4155/1/tests/metadata/test_ddl.py
File tests/metadata/test_ddl.py:

PS1, Line 234:     assert "ddl_test_db" not in self.all_db_names()
(I found this by searching for the removed members of TEST_DBS.)

  assert unique_database not in self.all_db_names()


PS1, Line 255:     unique_dabatase2 = unique_database + '2'
             :     self._create_db(unique_dabatase2, sync=True)
Clever, but it's unfortunate you have to do this. I'm not aware of pytest being able to use the same fixture twice (or more) in a test.

Should we parametrize unique_database optionally to return a set of databases instead of just one? It seems like it could use your incrementing numerical suffix scheme and be fine.


PS1, Line 328:     create_fn_stmt = "create function {0}.f() returns int "\
             :                      "location '{1}/libTestUdfs.so' symbol='NoArgs'"\
             :                      .format(unique_database, WAREHOUSE)
Tip for here and elsewhere: You can join strings across lines without \ if they are within parens.

    create_fn_stmt = ("create function {0}.f() returns int "
                      "location '{1}/libTestUdfs.so' symbol='NoArgs'"
                      .format(unique_database, WAREHOUSE))


http://gerrit.cloudera.org:8080/#/c/4155/1/tests/metadata/test_hms_integration.py
File tests/metadata/test_hms_integration.py:

Line 55:   def test_sanity(self, vector):
Not your changes, but in case you want to improve: This test doesn't have many asserts. I think it could be improved by replacing the self.client.execute() statements with self.execute_query_expect_success() statement. L70 for sure is never verified.


-- 
To view, visit http://gerrit.cloudera.org:8080/4155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf667dd5e960768879c019e2037cf48ad4e4241b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-HasComments: Yes