David Knupp has posted comments on this change. Change subject: IMPALA-1683: Allow REFRESH on a single partition ......................................................................
Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/3385/6/tests/metadata/test_refresh_partition.py File tests/metadata/test_refresh_partition.py: PS6, Line 16: import logging : import pytest Removed unused imports PS6, Line 19: import shlex Unused import PS6, Line 21: import subprocess Unused import PS6, Line 23: import * "import *" is generally regarded as bad practice in python. It's better to import symbols by name. PS6, Line 52: class ImpalaTableWrapper(object): Sorry -- I didn't notice before that this class is nested within the outer test class. I'm not sure that's really necessary here. Unless there's a good reason to do, can you move this class so that it's scoped at the same level as your TestRefreshPartition class? Line 82: for row in rows: This is perfectly well formed, although it would have been also possible to write it as a python one liner using a list comprehension: return [row.split('\t')[0:-8] for row in rows] That's a very common pattern. But that said, since can't tell what single row looks like, the slice [0:-8] looks odd to me. How long is the original row? How many tokens after splitting on '\t'? What are the 8 elements you're chopping off? How many elements does that leave remaining? In other words, I know that each "name" is a list of strings, but I can't tell from reading this code how long the list is, or what each element in the resulting list "name" is. I just know that name is eight elements shorter than row.split('\t'). PS6, Line 99: range(0, 16) You actually don't need the 0, and in python 2.x, it's more customary to use xrange. ... for i in xrange(16) -- To view, visit http://gerrit.cloudera.org:8080/3385 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4bf5dd630f2e175be5800187bbf9337c91f82bd8 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
