Matthew Jacobs has posted comments on this change.

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

Patch Set 1:

File testdata/bin/

PS1, Line 196: primay
File tests/common/

does this need to be its own file?
File tests/common/

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.
File tests/custom_cluster/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

Reply via email to