impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <>
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:

File tests/metadata/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I4bf5dd630f2e175be5800187bbf9337c91f82bd8
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Bikramjeet Vig <>
Gerrit-Reviewer: Bikramjeet Vig <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-HasComments: Yes

View raw message