impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-1683: Allow REFRESH on a single partition
Date Tue, 12 Jul 2016 17:41:00 GMT
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 <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message