-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding reversed scanner, and new compat mode 0.98 #155
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, this looks rather good! i requested a few changes using comments at the appropriate places. then it should be ready for merging.
@@ -277,6 +282,9 @@ def scan(self, row_start=None, row_stop=None, row_prefix=None, | |||
* The `sorted_columns` argument is only available when using | |||
HBase 0.96 (or up). | |||
|
|||
* The `reversed` argument is only available when using HBase 0.98 | |||
(or up) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: the period at the end of the sentence is missing
@@ -222,7 +222,7 @@ def cells(self, row, column, versions=None, timestamp=None, | |||
def scan(self, row_start=None, row_stop=None, row_prefix=None, | |||
columns=None, filter=None, timestamp=None, | |||
include_timestamp=False, batch_size=1000, scan_batching=None, | |||
limit=None, sorted_columns=False): | |||
limit=None, sorted_columns=False, reversed=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a
.. versionadded:: TODO
placeholder?
@@ -222,7 +222,7 @@ def cells(self, row, column, versions=None, timestamp=None, | |||
def scan(self, row_start=None, row_stop=None, row_prefix=None, | |||
columns=None, filter=None, timestamp=None, | |||
include_timestamp=False, batch_size=1000, scan_batching=None, | |||
limit=None, sorted_columns=False): | |||
limit=None, sorted_columns=False, reversed=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason you used reversed
, not reverse
? python itself uses reverse
for this purpose, e.g. sorted(, ...reverse=...)
and list.sort(reverse=True)
. reversed()
is the name of the built-in function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, I had forgotten about the built-in reversed
. As it stands today, filter argument also shadows built-in filter
though.
@@ -376,6 +393,7 @@ def scan(self, row_start=None, row_stop=None, row_prefix=None, | |||
filterString=filter, | |||
batchSize=scan_batching, | |||
sortColumns=sorted_columns, | |||
reversed=reversed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing comma please
@@ -446,6 +446,31 @@ def test_scan_sorting(): | |||
list(row.items())) | |||
|
|||
|
|||
def test_scan_reversed(): | |||
if connection.compat < '0.98': | |||
return # not supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you change this to expect a NotImplementedError
instead in this case?
@wbolster changes have been committed. thanks for the review. |
thanks, this looks good. merging. |
#67 implementing the reversed scan found in HBase 0.98.
I did not add a
.. versionadded:
to the docs, waiting to see if the PR will be accepted, and which version number to add.