impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-1878: Support INSERT and LOAD DATA on S3 and between filesystems
Date Sat, 30 Apr 2016 02:29:21 GMT
Sailesh Mukil has posted comments on this change.

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


Patch Set 27:

(9 comments)

Thanks Marcel.

http://gerrit.cloudera.org:8080/#/c/2574/27/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

Line 400:   // Fully qualified URI with the scheme to the base directory for this partition.
> what does "with the scheme to the base directory" mean?
it was actually supposed to be "URI with the scheme, to the base directory". But I forgot
to remove "with the scheme" when I added "Fully qualified". Changed it now.


http://gerrit.cloudera.org:8080/#/c/2574/27/fe/src/main/java/com/cloudera/impala/analysis/LoadDataStmt.java
File fe/src/main/java/com/cloudera/impala/analysis/LoadDataStmt.java:

Line 153:               "INPATH location '%s' cannot contain non-hidden subdirectories.",
sourceDataPath_));
> long line (i know, not your fault)
Done


http://gerrit.cloudera.org:8080/#/c/2574/27/fe/src/main/java/com/cloudera/impala/common/FileSystemUtil.java
File fe/src/main/java/com/cloudera/impala/common/FileSystemUtil.java:

Line 88:   private static boolean arePathsInSameEncryptionZone(FileSystem fs, Path p1,
> arePathsInSameHdfsEncryptionZone?
Done


Line 149:    * preserving the the existing file extension. If renameIfAlreadyExists is false,
an
> the the
Done


Line 157:     Path destFile = destFs.isDirectory(dest) ? new Path(dest, sourceFile.getName())
:
> better:
Done


Line 167:     boolean isDestDfs = isDistributedFileSystem(destFs);
> destIsDfs
Done


Line 189:       LOG.info(String.format(
> precede w/ preconditions.checkstate(!doRename))
Done


Line 193:       LOG.info(String.format("Copying '%s' to '%s' between filesystems.",
> couldn't this be !isDestDfs && sameFileSystem?
No, that would always make doRename = true, and so this code wouldn't get executed. Do you
think I should make the else into an else if() so it's more readable?


Line 286:    * Returns true iff the filesystem is a S3AFileSystem.
> if the path is on an ...
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/2574
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I94e15ad67752dce21c9b7c1dced6e114905a942d
Gerrit-PatchSet: 27
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message