[Impala-CR](cdh5-trunk) IMPALA-3530: Clean up test_ddl.py. Part 1.

["Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>]

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-3530: Clean up test_ddl.py. Part 1.
......................................................................


Patch Set 2:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/3044/2//COMMIT_MSG
Commit Message:

Line 14: create_table
I presume Kudu and HBase will have their own test files? How about tables that live on S3? Do these test files cover these cases? If not, it may make sense to change the naming so that it reflects what exactly is tested in each file; maybe test_create_hdfs_table or something along these lines.


http://gerrit.cloudera.org:8080/#/c/3044/2/testdata/workloads/functional-query/queries/QueryTest/create-database.test
File testdata/workloads/functional-query/queries/QueryTest/create-database.test:

Line 86: # Should drop all tables and the database
Shouldn't also drop functions and aggregate functions?


Line 95: create database if not exists test_drop_restrict_db
       : ====
       : ---- QUERY
       : show databases like 'test_drop_restrict_db'
       : ---- RESULTS
       : 'test_drop_restrict_db',''
       : ---- TYPES
       : STRING,STRING
       : ====
       : ---- QUERY
       : drop database test_drop_restrict_db restrict
       : ---- RESULTS
       : ====
       : ---- QUERY
       : show databases like 'test_drop_restrict_db'
       : ---- RESULTS
What are we testing here?


http://gerrit.cloudera.org:8080/#/c/3044/2/testdata/workloads/functional-query/queries/QueryTest/create-table-as-select.test
File testdata/workloads/functional-query/queries/QueryTest/create-table-as-select.test:

Line 98: # Validate CTAS with LIMIT 0
       : create table if not exists ctas_join_limit0 stored as textfile as
       : select * from functional.jointbl limit 0
Duplicate of test in L73?


http://gerrit.cloudera.org:8080/#/c/3044/2/testdata/workloads/functional-query/queries/QueryTest/create-table-like-table.test
File testdata/workloads/functional-query/queries/QueryTest/create-table-like-table.test:

Line 59: show table stats like_view
Do these statements belong here? Maybe putting them in the compute-stats tests?


http://gerrit.cloudera.org:8080/#/c/3044/2/testdata/workloads/functional-query/queries/QueryTest/create-table.test
File testdata/workloads/functional-query/queries/QueryTest/create-table.test:

Line 3: # It should show up now
Can you update the comment?


Line 45: insert overwrite table ddl_test_db.testtbl SELECT 1, 'Hi'
       : from functional.alltypes limit 10
I am not sure DML statements should be in this or any of the create table like test files. What I expect from this file is testing all possible ways to call create table and then doing some simple verification to ensure that: 1) the tables are successfully created and 2) that the specified properties are applied.


Line 226: creatin
"creation"


http://gerrit.cloudera.org:8080/#/c/3044/2/tests/metadata/test_ddl_base.py
File tests/metadata/test_ddl_base.py:

Line 23: TEST_DBS = ['ddl_test_db', 'ddl_purge_db', 'alter_table_test_db',
       :               'alter_table_test_db2', 'function_ddl_test', 'udf_test', 'data_src_test',
       :               'truncate_table_test_db', 'test_db', 'alter_purge_db', 'db_with_comment']
Why does this belong to the super class? I would expect every subclass to define the TEST_DBS that it uses.


Line 60: 'part_data', 't1_tmp1', 't_part_tmp'
This looks specific to some tests, so I am not sure it belongs here.


Line 66:  
.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5f4c044d39e165c2535961b8d0a765c8dbbd051c
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: Yes