impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5333: Add support for Impala to work with ADLS
Date Thu, 18 May 2017 19:18:35 GMT
David Knupp has posted comments on this change.

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

Patch Set 2:

File tests/util/

Line 29:   @classmethod
This seems really unusual to me. __init__ methods typically don't need a @classmethod decorator,
since what you're initializing is an instance of the class, not the class itself. By adding
the decorator, "self" here becomes a reference to the class, not the instance.

Perhaps there's something more exotic going on here that I'm not picking up on though. Let
me know if I'm off target with this. I've just never seen this done.

PS2, Line 40: f.close()
> I believe this is redundant but I could be wrong.
You're correct -- "with  ... as foo" should close foo for you.

Line 45:     self.adlsclient.mkdir(path)
What is the return value of self.adlsclient.mkdir(path)? Does it make sense to return it instead?
What happens if the call fails?

Line 72:     files =
Super insignificant nit: you could put this inside of the list comprehension to make this
method a tidy one-liner.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic56b9988b32a330443f24c44f9cb2c80842f7542
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Attila Jeges <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message