impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
Date Mon, 19 Sep 2016 20:35:55 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4414/1/testdata/bin/generate-schema-statements.py
File testdata/bin/generate-schema-statements.py:

PS1, Line 196: primay
primary


http://gerrit.cloudera.org:8080/#/c/4414/1/tests/common/__init__.py
File tests/common/__init__.py:

does this need to be its own file?


http://gerrit.cloudera.org:8080/#/c/4414/1/tests/common/kudu_test_suite.py
File tests/common/kudu_test_suite.py:

PS1, Line 54:   @classmethod
            :   def get_db_name(cls):
            :     # When py.test runs with the xdist plugin, several processes are started
and each
            :     # process runs some partition of the tests. It's possible that multiple
processes
            :     # will call this method. A random value is generated so the processes won't
try
            :     # to use the same database at the same time. The value is cached so within
a single
            :     # process the same database name is always used for the class. This doesn't
need to
            :     # be thread-safe since multi-threading is never used.
            :     if not cls.__DB_NAME:
            :       cls.__DB_NAME = \
            :           choice(ascii_lowercase) + "".join(sample(ascii_lowercase + digits,
5))
            :     return cls.__DB_NAME
I've always disliked this function... I think we should try to use the db fixture that Michael
Brown added. Maybe I'm missing something though. It'd be good to get Michael's input here.


PS1, Line 127:     mapping = {BOOL: "BOOLEAN",
             :         DOUBLE: "DOUBLE",
             :         FLOAT: "FLOAT",
             :         INT16: "SMALLINT",
             :         INT32: "INT",
             :         INT64: "BIGINT",
             :         INT8: "TINYINT",
             :         STRING: "STRING"}
this gets defined as a local every function invocation, right? how about creating the map
outside the fn so its static.


http://gerrit.cloudera.org:8080/#/c/4414/1/tests/custom_cluster/test_kudu.py
File tests/custom_cluster/test_kudu.py:

PS1, Line 25: TestRedaction
?


PS1, Line 42: temp_kudu_table
I'd think we could have the test fn take the database fixture and then pass it to temp_kudu_table


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message