Csaba Ringhofer has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13785 )

Change subject: IMPALA-8734: Reload table schema on TBLPROPERTIES change
......................................................................


Patch Set 3:

(3 comments)

I wrote some comments for the test, but probably the simplest way would be to 
move the whole logic to alter_table.test. Some strings could be inserted to the 
table, check select *, then serialization.null.format could be set to one of 
the values, then check select * again.

http://gerrit.cloudera.org:8080/#/c/13785/3/tests/metadata/test_ddl.py
File tests/metadata/test_ddl.py:

http://gerrit.cloudera.org:8080/#/c/13785/3/tests/metadata/test_ddl.py@697
PS3, Line 697:     # IMPALA-8734: Force a table schema reload when setting 
table properties.
Is there a reason for not creating a new test? The new code seems to be 
completely independent from the earlier things in 
test_create_alter_tbl_properties


http://gerrit.cloudera.org:8080/#/c/13785/3/tests/metadata/test_ddl.py@702
PS3, Line 702: null
Using a less "null like" value could make the test's intention clearer - it was 
not obvious for me that "null" is not interpreted as NULL by default.


http://gerrit.cloudera.org:8080/#/c/13785/3/tests/metadata/test_ddl.py@701
PS3, Line 701:     temp_file, temp_path = tempfile.mkstemp()
             :     os.write(temp_file, "\nnull\n")
             :     os.close(temp_file)
             :     subprocess.check_call(["hdfs", "dfs", "-put", "-f", 
temp_path,
             :                            
"/test-warehouse/{0}.db/{1}/f".format(unique_database,
             :                                                                  
tbl_name)])
             :     self.execute_query_expect_success(self.client,
             :                                       "alter table {0}.{1} set 
tblproperties"
             :                                       
"('serialization.null.format'='null')"
             :                                       .format(unique_database, 
tbl_name))
First I thought that it would be nice to create a utility function for this, 
then I realized that there is already one: 
https://github.com/apache/impala/blob/aee0abd76b762e57ce9f0a2e40a9a8b4f97dc986/tests/util/hdfs_util.py#L172



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a43a962c2a456f3ddc078b2924f551fccb5c2ad
Gerrit-Change-Number: 13785
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <[email protected]>
Gerrit-Reviewer: Bharath Vissapragada <[email protected]>
Gerrit-Reviewer: Csaba Ringhofer <[email protected]>
Gerrit-Reviewer: Fredy Wijaya <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Comment-Date: Tue, 02 Jul 2019 17:50:33 +0000
Gerrit-HasComments: Yes

Reply via email to