-
Notifications
You must be signed in to change notification settings - Fork 2k
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
DynamoDB Reminders load using GSI #7437
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -146,31 +146,48 @@ public async Task<ReminderTableData> ReadRows(GrainReference grainRef) | |
/// <summary> | ||
/// Reads reminder table data for a given hash range. | ||
/// </summary> | ||
/// <param name="beginHash"></param> | ||
/// <param name="endHash"></param> | ||
/// <param name="begin"></param> | ||
/// <param name="end"></param> | ||
/// <returns> Return the RemiderTableData if the rows were read successfully </returns> | ||
public async Task<ReminderTableData> ReadRows(uint beginHash, uint endHash) | ||
public async Task<ReminderTableData> ReadRows(uint begin, uint end) | ||
{ | ||
var expressionValues = new Dictionary<string, AttributeValue> | ||
{ | ||
{ $":{SERVICE_ID_PROPERTY_NAME}", new AttributeValue(this.serviceId) }, | ||
{ $":Begin{GRAIN_HASH_PROPERTY_NAME}", new AttributeValue { N = beginHash.ToString() } }, | ||
{ $":End{GRAIN_HASH_PROPERTY_NAME}", new AttributeValue { N = endHash.ToString() } } | ||
}; | ||
Dictionary<string, AttributeValue> expressionValues = null; | ||
|
||
try | ||
{ | ||
string expression = string.Empty; | ||
if (beginHash < endHash) | ||
List<ReminderEntry> records; | ||
|
||
if (begin < end) | ||
{ | ||
expression = $"{SERVICE_ID_PROPERTY_NAME} = :{SERVICE_ID_PROPERTY_NAME} AND {GRAIN_HASH_PROPERTY_NAME} > :Begin{GRAIN_HASH_PROPERTY_NAME} AND {GRAIN_HASH_PROPERTY_NAME} <= :End{GRAIN_HASH_PROPERTY_NAME}"; | ||
expressionValues = new Dictionary<string, AttributeValue> | ||
{ | ||
{ $":{SERVICE_ID_PROPERTY_NAME}", new AttributeValue(this.serviceId) }, | ||
{ $":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}"; | ||
records = await this.storage.QueryAllAsync(this.options.TableName, expressionValues, expression, this.Resolve, SERVICE_ID_INDEX, consistentRead: false).ConfigureAwait(false); | ||
} | ||
else | ||
{ | ||
expression = $"{SERVICE_ID_PROPERTY_NAME} = :{SERVICE_ID_PROPERTY_NAME} AND ({GRAIN_HASH_PROPERTY_NAME} > :Begin{GRAIN_HASH_PROPERTY_NAME} OR {GRAIN_HASH_PROPERTY_NAME} <= :End{GRAIN_HASH_PROPERTY_NAME})"; | ||
} | ||
expressionValues = new Dictionary<string, AttributeValue> | ||
{ | ||
{ $":{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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, the original query had this right: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see now. That's an odd limitation, but nevertheless this seems fine. |
||
records = await this.storage.QueryAllAsync(this.options.TableName, expressionValues, expression, this.Resolve, SERVICE_ID_INDEX, consistentRead: false).ConfigureAwait(false); | ||
|
||
var records = await this.storage.ScanAsync(this.options.TableName, expressionValues, expression, this.Resolve).ConfigureAwait(false); | ||
expressionValues = new Dictionary<string, AttributeValue> | ||
{ | ||
{ $":{SERVICE_ID_PROPERTY_NAME}", new AttributeValue(this.serviceId) }, | ||
{ $":Begin{GRAIN_HASH_PROPERTY_NAME}", new AttributeValue { N = begin.ToString() } } | ||
}; | ||
expression = $"{SERVICE_ID_PROPERTY_NAME} = :{SERVICE_ID_PROPERTY_NAME} AND {GRAIN_HASH_PROPERTY_NAME} > :Begin{GRAIN_HASH_PROPERTY_NAME}"; | ||
records.AddRange(await this.storage.QueryAllAsync(this.options.TableName, expressionValues, expression, this.Resolve, SERVICE_ID_INDEX, consistentRead: false).ConfigureAwait(false)); | ||
|
||
} | ||
|
||
return new ReminderTableData(records); | ||
} | ||
|
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.
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), butBETWEEN
is equivalent to=> Begin && <= End
(inclusive bounds)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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be
begin + 1
, rather thanend - 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 behaviorThere 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.
yes, thank you - vacation brain hasn't recovered yet.
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.
No sweat, I'm in the same boat
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.
Of course... it's impossible for both
begin < end
andbegin == uint.MaxValue
to be true at the same time, so theunchecked
is entirely meaningless as no overflow is possible 😅