DynamoDB Reminders load using GSI #7437
Conversation
|
|
| { $":Begin{GRAIN_HASH_PROPERTY_NAME}", new AttributeValue { N = begin.ToString() } }, | ||
| { $":End{GRAIN_HASH_PROPERTY_NAME}", new AttributeValue { N = end.ToString() } } | ||
| }; | ||
| expression = $"{SERVICE_ID_PROPERTY_NAME} = :{SERVICE_ID_PROPERTY_NAME} AND {GRAIN_HASH_PROPERTY_NAME} BETWEEN :Begin{GRAIN_HASH_PROPERTY_NAME} AND :End{GRAIN_HASH_PROPERTY_NAME}"; |
There was a problem hiding this comment.
Please correct me if I'm wrong, but I believe this changes the query semantics. It's supposed to be > Begin && <= End (non-inclusive lower bound, inclusive upper), but BETWEEN is equivalent to => Begin && <= End (inclusive bounds)
There was a problem hiding this comment.
I think you're correct. Since performing a dynamodb query with an AND is not possible, would it be sufficient to pass the end attribute value as end-1 and still use BETWEEN?
There was a problem hiding this comment.
@ReubenBond is correct. The query semantics are changed. I've overlooked this. The original one was correct. Are you sure this passed the unit tests? I remember the reminder tests being very nitpick about this.
There was a problem hiding this comment.
I missed an unrelated change in DynamoDBStorage that has now been corrected.
AWUtils.Tests.RemindersTest passes with the questionable code as-is, and also if I change line 167 to (end - 1).ToString(), which I believe is the correct change to make.
There was a problem hiding this comment.
Shouldn't it be begin + 1, rather than end - 1? The lower bound is supposed to be exclusive, not the upper bound.
It will work but please wrap the expression in unchecked(...) , since it's possible for it to overflow, just to make it extra obvious that it's expected behavior
There was a problem hiding this comment.
yes, thank you - vacation brain hasn't recovered yet.
There was a problem hiding this comment.
No sweat, I'm in the same boat
There was a problem hiding this comment.
Of course... it's impossible for both begin < end and begin == uint.MaxValue to be true at the same time, so the unchecked is entirely meaningless as no overflow is possible 😅
| { $":{SERVICE_ID_PROPERTY_NAME}", new AttributeValue(this.serviceId) }, | ||
| { $":End{GRAIN_HASH_PROPERTY_NAME}", new AttributeValue { N = end.ToString() } } | ||
| }; | ||
| expression = $"{SERVICE_ID_PROPERTY_NAME} = :{SERVICE_ID_PROPERTY_NAME} AND {GRAIN_HASH_PROPERTY_NAME} <= :End{GRAIN_HASH_PROPERTY_NAME}"; |
There was a problem hiding this comment.
This query should be <= End || > Begin (since "end" is smaller than "begin", it's a wrap-around query).
Here's a graphical depiction of the two queries, where a circle represents an exclusive bound and a bar represents an inclusive bound:
There was a problem hiding this comment.
Indeed, the original query had this right: > Begin or <= End: it's a conjunction of two queries
There was a problem hiding this comment.
Originally, this was performing a scan. I don't believe you can perform a query with the "or" clause, and thus I split the scan into two queries, then combine the results.
There was a problem hiding this comment.
Oh, I see now. That's an odd limitation, but nevertheless this seems fine.
|
Needs to be ported to |
* DynamoDB Reminders load using GSI * Update DynamoDBStorage.cs * Update DynamoDBReminderTable.cs * Update DynamoDBReminderTable.cs Co-authored-by: Reuben Bond <203839+ReubenBond@users.noreply.github.com>
* DynamoDB Reminders load using GSI * Update DynamoDBStorage.cs * Update DynamoDBReminderTable.cs * Update DynamoDBReminderTable.cs Co-authored-by: Reuben Bond <203839+ReubenBond@users.noreply.github.com> Co-authored-by: michaeltdaniels <45430678+michaeltdaniels@users.noreply.github.com>
Fixes #7434
Microsoft Reviewers: Open in CodeFlow