Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11127 )
Change subject: IMPALA-6709: Simplify tests that copy local files to tables ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/11127/2/tests/common/test_files.py File tests/common/test_files.py: http://gerrit.cloudera.org:8080/#/c/11127/2/tests/common/test_files.py@1 PS2, Line 1: > About the name of the file: test_* is a conventional way to name files that There are other utility files in common/ that start with test. Anyway, I can agree with your point, renamed it to file_utils.py http://gerrit.cloudera.org:8080/#/c/11127/2/tests/query_test/test_parquet_stats.py File tests/query_test/test_parquet_stats.py: http://gerrit.cloudera.org:8080/#/c/11127/2/tests/query_test/test_parquet_stats.py@64 PS2, Line 64: t > I would prefer to start these without '/'. I removed the '/' from the beginning because it suggests that it is a relative path. I think assuming $IMPALA_HOME only is more straightforward. Seeing '../' in the path would be quite confusing I think. http://gerrit.cloudera.org:8080/#/c/11127/1/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/11127/1/tests/query_test/test_scanners.py@a318 PS1, Line 318: : > I was confused by some of our documentation that indicates TINYINT/SMALLINT Thanks! http://gerrit.cloudera.org:8080/#/c/11127/2/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/11127/2/tests/query_test/test_scanners.py@307 PS2, Line 307: self.clien > Just an idea to make these shorter: this functions could be ImpalaTestSuite If there is no strong preference for it, I'd rather leave those as free functions for now. ImpalaTestSuite already does too many things I think. -- To view, visit http://gerrit.cloudera.org:8080/11127 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie00a4561825facf8abe2e8e74a6b6e93194f416f Gerrit-Change-Number: 11127 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 13 Aug 2018 09:21:41 +0000 Gerrit-HasComments: Yes
