Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

dynamodb to use Decimal to validate/store/retrieve numbers #1060

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

disruptek commented Oct 15, 2012

This is lightly tested; mostly a proof-of-concept for peer review in issue #890, but it does assert the correct precision requirements and perform lossless read/write.

Owner

jamesls commented Oct 22, 2012

I'm trying out these changes, looks good. Just a few comments:

Based on the discussion in #890, it looks like we're explicitly breaking lossy float conversions. While I agree that this is the safest choice (as we won't be implicity storing a different value that the user specified) I am sympathetic to the code we'd be breaking:

item['floats'] = set([1.1, 2.2, 3.3, 4.4, 5.5])
item.put()

This is taken from one of the dynamodb integration tests (tests/integration/dynamodb/test_layer2.py:test_layer2_basic) which now fails as expected. Perhaps we could give a helpful error message telling the user what to do to fix this.

#890 mentioned that we might be able to provide some sort of config option/hook that allow users to have minimal impact to their code until they can update to using decimals. @garnaat thoughts on adding this mechanism?

We should probably also add some tests for this new functionality.

@jamesls jamesls commented on the diff Oct 22, 2012

boto/dynamodb/types.py
@@ -41,12 +44,26 @@ def is_binary(n):
return isinstance(n, Binary)
+dynamodb_context = Context(Emin=-128, Emax=126, rounding=None, prec=38,
+ traps=[Clamped, Overflow, Inexact, Rounded, Underflow])
+
+def serialize_num(s):
@jamesls

jamesls Oct 22, 2012

Owner
  • Isn't L56 the same as if n in ('Infinity', 'Nan')
  • Perhaps we should catch DecimalException so the TypeError can propogate?
@disruptek

disruptek Oct 22, 2012

Contributor

The reason to catch the TypeError is to wrap it in the same DynamoDBNumberError exception, so that all our number validation yields a uniform exception type to the user.

@jamesls

jamesls Oct 23, 2012

Owner

Ok, that makes sense, thanks for the clarification (though I'd still prefer to catch something more specific than Exception (e.g. except (DecimalException, TypeError)).

Contributor

disruptek commented Oct 23, 2012

Okay, so per IRC discussion we want a variable on Item that defaults to 'weak', which logs warnings at lossy conversion, while the 'strict' value causes exceptions. I'll also tweak the exception handling as per your comment above.

Regarding the question of how to handle values retrieved from DynamoDB, we could take this opportunity to allow the user to specify the value's type. One problem with this is how to specify the old behavior of floats when a period is present and integers otherwise. A type value of None might serve as a special indication that the user wants the original behavior without complicating things with another type.

It then follows that we could simply allow this type variable to also serve as our weak/strict flag; setting the variable to Decimal would cause Decimal I/O while leaving it as None would yield the original behavior, albeit with warnings. Of course, this feature would also allow the user to easily specify their own type for custom number handling.

Owner

jamesls commented Jan 7, 2013

Closing in favor of #1183, which supersedes this.

@jamesls jamesls closed this Jan 7, 2013

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