impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-2904: Support INSERT and LOAD DATA on S3 and between filesystems
Date Tue, 22 Mar 2016 20:21:44 GMT
Sailesh Mukil 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:
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:

Line 398:     assert new_ls == ls
> assert ls == new_ls == 1
But we assert the len(new_ls) above. So, assert ls == new_ls == 1 would fail.
File tests/util/

Line 25: file_data
> is file_data a string?
Yes, I've updated to explicitly call it 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 permiss
I meant to take this out. 755 is default for the parent too. So, this doesn't change anything.

Line 102:   def ls(self, path):
> Can you add a more detailed comment? What is the difference between this me
We want ls() to return only the file names instead of a dictionary of file statuses. list_dir()
returns the latter, which we don't need anywhere in our tests save for one or 2 cases. Done.
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 re

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:

Line 75:       key = tmp[-1]
> how about simply:

Line 83:     return contents is not None
> how about return response.get('Contents') is not None

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?
No, the 'path' itself will be one of the keys returned by list_keys().

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: Mostafa Mokhtar <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message