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

["Alex Behm (Code Review)" <ge...@cloudera.org>]

Alex Behm 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:

PS2, Line 14: create_table
> I presume Kudu and HBase will have their own test files? How about tables t
We cannot create HBase tables.
S3 tables are 'HDFS' tables with special hadoop configurations, but no changes to the DDL. Same for localfilesystem tables.

Kudu create tests are separate.

The most accurate name seems to be test_create_non_kudu_tables() which seems weird.

I added a TODO to move the Kudu-specific stuff into a separate pytest. Right now the Kudu tests are spread in various places, and I want to avoid making too many changes in this patch, if possible.


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?
Correct. Modified comment.


PS2, 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?
Tests that a drop with RESTRICT executes fine. Added comment.


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:

PS2, 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?
Hard to tell. It may have a purpose, e.g., to test the behavior when the target
table already exists. I verified that I did not mistakenly add the duplicate, tests were like
this before my patch. I added a comment to clarify the distinction.


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:

PS2, Line 59: show table stats like_view
> Do these statements belong here? Maybe putting them in the compute-stats te
This is testing that the table is created in an expected state with "create table like".
We could move that to compute-stats.test, but it's not really testing compute stats.


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:

PS2, Line 3: # It should show up now
> Can you update the comment?
Done


PS2, 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 l
1. Do you want me to remove those out-of-place statements (here and elsewhere) and lose test coverage?
2. There have been cases in the past where creating a table worked, but performing operations against it did not. I think what we really want to test is that table creation in all possible ways works, that the table is created in an expecte state, and that all possible operations against that table also work (i.e. the table is usable).
3. The ultimate goal is to enable the unique_database fixture to speed up tests.


PS2, Line 226: creatin
> "creation"
Done


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

PS2, 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 d
Moved the setup/teardown parts into the only subclass. I will split them up later if needed.


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


PS2, Line 66:  
> .
Done


-- 
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