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 <dtsirogian...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
