impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5333: Add support for Impala to work with ADLS
Date Wed, 17 May 2017 22:09:00 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5333: Add support for Impala to work with ADLS

Patch Set 1:

Commit Message:

PS1, Line 18: This client seems to have some consistency
            : issues.
Is this a known issue? Any tickets to reference for this?
File fe/src/main/java/org/apache/impala/catalog/

PS1, Line 774: S3
File infra/python/deps/compiled-requirements.txt:

Line 35:   pycparser == 2.17
IIRC the indentation is meant to indicate dependency. In the Kudu case below, the comment
is meant to be the 'top level'. Just leave a comment above this indicating # Required for
File tests/metadata/

PS1, Line 61: seems
It'd be good to make a conclusive statement. I assume the consistency model should be well
defined somewhere.

nit: "so skip for it" sounds awkward

Line 156:       print

Line 304:       # The ADLS client seems to be inconsistent too. So skip for it.
same as above
File tests/metadata/

PS1, Line 22: SkipIfADLS
not used
File tests/query_test/

Line 48:   @SkipIfADLS.slow_client
why does this work on s3 but not adls?
File tests/query_test/

Line 164:   @SkipIfADLS.hdfs_block_size
does this work on s3?
File tests/util/

Line 26: class ADLSClient(BaseFilesystem):
nit: newline above this

Line 38:       f.write(file_data)
this has a return value, #bytes I think, it'd be good to check, e.g. log an error/return false
if it is less than expected.

PS1, Line 47:     # The ADLS Python client doesn't support cp() yet, so we have to download
            :     # reupload to the destination.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic56b9988b32a330443f24c44f9cb2c80842f7542
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

View raw message