impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-2904: Support INSERT and LOAD DATA on S3 and between filesystems
Date Tue, 22 Mar 2016 02:24:32 GMT
Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-2904: Support INSERT and LOAD DATA on S3 and between filesystems

Patch Set 6:

File tests/common/

Line 118:     if IS_S3:
Much shorter and clearer:
cls.filesystem_client = S3Client(S3_BUCKET_NAME) if IS_S3 else cls.hdfs_client
File tests/query_test/

Line 110:     ls =
I don't think we need to define the ls variable here, just assert directly

Line 358:       does_exist = self.filesystem_client.exists(path)
how about:
assert self.filesystem_client.exists(path) == should_exist

Line 398:     assert new_ls == ls
assert ls == new_ls == 1
File tests/util/

Line 25: file_data
is file_data a string?
File tests/util/

Line 59:   def make_dir(self, path, permission=755):
Do we need to override this? Is the only purpose to set the default permission to 755? Is
this the default for the parent class?

Line 102:   def ls(self, path):
Can you add a more detailed comment? What is the difference between this method and list_dir?
Maybe describe the use case for this because it's not obvious to me what the purpose of this
method is.
File tests/util/

Line 15: # S3 access utilities
This comment does not really add anything.

Line 22:   @classmethod
Add an empty line above this line.

Line 30:     if not overwrite:
if not override and self.exists(filename): return False

Line 33: response
What's the point of this variable, it just disappears? I think we should return true only
if the operation succeeded

Line 37: othav
add space

Line 57: =
path += '/'

Line 69: both
This should be called files_and_dirs

Line 70:     for d in dirs:
How about something like:
files_and_dirs.extend([d.split('/')[-2] for d in dirs])

Line 75:       key = tmp[-1]
how about simply:
key = f.split('/')[-1]

This avoids creating the unnecessary tmp variable.

Line 83:     return contents is not None
how about return response.get('Contents') is not None
to avoid creating an extra variable

Line 90:       keys = self.list_keys(path)
objects = [{'Key': k} for k in self.list_keys(path)] if recursive else path

Line 92:         objects.append({'Key':k})
Do we have to add path to the list as well in the recursive case?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I94e15ad67752dce21c9b7c1dced6e114905a942d
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message