DynamoDB2: Table.query() reverse parameter is misleading #2070

Closed
tsavola opened this Issue Feb 4, 2014 · 2 comments

Comments

Projects
None yet
5 participants
@tsavola
Contributor

tsavola commented Feb 4, 2014

While the documentation doesn't explicitly specify what the normal ordering of the results is, the actual operation is counterintuitive: the "reverse" parameter is mapped to the ScanIndexForward parameter of the underlying Amazon DynamoDB API. Also the default value doesn't match documentation (False vs. None). In other words, if I call the query method without specifying "reverse" or set it to False, ScanIndexForward will be false, and I get results in descending order. If I set "reverse" to True, ScanIndexFoward will be true, and I get results in ascending order.

Since the default order is descending, simply deprecating the "reverse" parameter and adding a new one with opposite meaning doesn't fix the situation completely. My suggestion is to deprecate the query method and add a new one (e.g. query2) with a parameter which maps directly to ScanIndexForward (e.g. "forward") or inversely (e.g. "reversed" or "descending").

@toastdriven toastdriven self-assigned this Feb 5, 2014

@chosak

This comment has been minimized.

Show comment Hide comment
@chosak

chosak Feb 19, 2014

Just ran into this same issue as well. Observed behavior is:

  • Don't specify parameter - default is reverse=False, although documentation erroneously says it will be set to None. This causes ScanIndexForward=false which returns the results in descending order (see AWS documentation here). This is counterintuitive. If I don't specify reverse, I expect results in the default, ascending order.
  • Set reverse=None. This omits ScanIndexForward from the API call, which returns results in the default, ascending order. Behavior as expected.
  • Set reverse=True. This sets ScanIndexForward=true which returns results in ascending order. This is wrong. If I specify reverse, I expect results in descending order.
  • Set reverse=False. This sets ScanIndexForward=false which returns results in descending order, the opposite of what I expect. This is also wrong.

In the previous version of the boto dynamo API, the table.query() method took a scan_index_forward parameter which is consistent with the underlying DynamoDB API. Using the reverse argument is easier to understand, if it works as expected.

One possible solution would be to just invert the parameter's value as specified in _query(), something like:

    scan_index_forward = not reverse if reverse is not None else None
    kwargs = {
        ...
        'scan_index_forward': scan_index_forward,
        ...
    }

chosak commented Feb 19, 2014

Just ran into this same issue as well. Observed behavior is:

  • Don't specify parameter - default is reverse=False, although documentation erroneously says it will be set to None. This causes ScanIndexForward=false which returns the results in descending order (see AWS documentation here). This is counterintuitive. If I don't specify reverse, I expect results in the default, ascending order.
  • Set reverse=None. This omits ScanIndexForward from the API call, which returns results in the default, ascending order. Behavior as expected.
  • Set reverse=True. This sets ScanIndexForward=true which returns results in ascending order. This is wrong. If I specify reverse, I expect results in descending order.
  • Set reverse=False. This sets ScanIndexForward=false which returns results in descending order, the opposite of what I expect. This is also wrong.

In the previous version of the boto dynamo API, the table.query() method took a scan_index_forward parameter which is consistent with the underlying DynamoDB API. Using the reverse argument is easier to understand, if it works as expected.

One possible solution would be to just invert the parameter's value as specified in _query(), something like:

    scan_index_forward = not reverse if reverse is not None else None
    kwargs = {
        ...
        'scan_index_forward': scan_index_forward,
        ...
    }
@jamesonjlee

This comment has been minimized.

Show comment Hide comment
@jamesonjlee

jamesonjlee Feb 21, 2014

Contributor

ran into this today, The document needs to be updated.

Contributor

jamesonjlee commented Feb 21, 2014

ran into this today, The document needs to be updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment