Skip to content
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

Migrate to expression attributes for Query and Scan #655

Merged

Conversation

andrykonchin
Copy link
Member

@andrykonchin andrykonchin commented Apr 21, 2023

Get rid of legacy Query/Scan's attributes.

Use FilterExpression, KeyConditionExpression and ProjectionExpression instead of legacy ScanFilter, QueryFiler and AttributesToGet attributes in Scan and Query operations.

Changes:

@andrykonchin andrykonchin changed the title Migrate to expression attributes for query and scan Migrate to expression attributes for Query and Scan Apr 21, 2023
@codecov
Copy link

codecov bot commented Apr 21, 2023

Codecov Report

Merging #655 (38541b1) into master (80bbd44) will increase coverage by 0.04%.
The diff coverage is 99.53%.

❗ Current head 38541b1 differs from pull request most recent head ceb1c37. Consider uploading reports for the commit ceb1c37 to get more accurate results

@@            Coverage Diff             @@
##           master     #655      +/-   ##
==========================================
+ Coverage   90.28%   90.33%   +0.04%     
==========================================
  Files          61       62       +1     
  Lines        3141     3157      +16     
==========================================
+ Hits         2836     2852      +16     
  Misses        305      305              
Impacted Files Coverage Δ
lib/dynamoid/criteria/chain.rb 99.54% <98.33%> (-0.05%) ⬇️
lib/dynamoid/adapter.rb 100.00% <100.00%> (ø)
lib/dynamoid/adapter_plugin/aws_sdk_v3.rb 94.30% <100.00%> (-0.18%) ⬇️
...r_plugin/aws_sdk_v3/filter_expression_convertor.rb 100.00% <100.00%> (ø)
...ugin/aws_sdk_v3/projection_expression_convertor.rb 100.00% <100.00%> (ø)
lib/dynamoid/adapter_plugin/aws_sdk_v3/query.rb 100.00% <100.00%> (ø)
lib/dynamoid/adapter_plugin/aws_sdk_v3/scan.rb 100.00% <100.00%> (ø)
lib/dynamoid/criteria/key_fields_detector.rb 100.00% <100.00%> (ø)
lib/dynamoid/criteria/where_conditions.rb 100.00% <100.00%> (ø)
lib/dynamoid/finders.rb 94.84% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@andrykonchin andrykonchin marked this pull request as draft April 21, 2023 18:58
@andrykonchin andrykonchin force-pushed the migrate-to-expression-attributes-for-query-and-scan branch from 22ddc25 to 9694b51 Compare April 22, 2023 12:53
@andrykonchin andrykonchin force-pushed the migrate-to-expression-attributes-for-query-and-scan branch from 9694b51 to ceb1c37 Compare April 22, 2023 13:13
@andrykonchin andrykonchin marked this pull request as ready for review April 22, 2023 13:13
@github-actions
Copy link

Code Coverage

Package Line Rate Health
dynamoid 90%
Summary 90% (2852 / 3157)

Minimum allowed line rate is 90%

@andrykonchin andrykonchin merged commit 6d757f1 into master Apr 22, 2023
48 of 49 checks passed
@andrykonchin andrykonchin deleted the migrate-to-expression-attributes-for-query-and-scan branch April 22, 2023 13:25
@nbulaj
Copy link

nbulaj commented Mar 25, 2024

Hi @andrykonchin 👋

Just tried to use multiple conditions on the same key:

Score.where("timestamp.gte": 1.day.ago).where("timestamp.lte": Time.now).count

And also got Aws::DynamoDB::Errors::ValidationException (KeyConditionExpressions must only contain one condition per key)

Is see the spec are skipped for it.. But maybe any info on the plans to support multiple conditions?

@andrykonchin
Copy link
Member Author

andrykonchin commented Apr 1, 2024

Thank you for reporting the issue.

So DynamoDB allows only one condition for a sort key. I've completely overlooked this restriction. Could you please describe it in a separate issue?

In this particular case timestamp.between could be a proper solution to express a now - 1 day < timestamp < now condition.

Generally speaking supporting multiple conditions for a sort key isn't as useful as it may seem. DynamoDB supports only subset of operations for searching (in KeyConditionExpression, that affects operation cost and performance) - >, >=, <, <=, between. All the other operators are used in filtering (using FilterExpression) that doesn't speed up an operation but reduces HTTP traffic - amount of data that will be send in response. (Actually Dynamoid doesn't distinguish them right now and all the operators are sent as search conditions - so it's a separate bug). So there might be no value in multiple < or > conditions. It will be just more convenient to build a complex condition. But still it makes sense to allow it.

The only issue with supporting multiple conditions for sort key is that only one of them will be used as a search condition and all the other will be used as filter conditions. This choice will affect performance and cost. For instance there are conditions a > 0 and a > 100. The second one is stricter but if the first one is used for searching and the second one for filtering - operation consumes more units than in the opposite case when the second one is used for searching. And this choice either makes Dynamoid (e.g. takes the first condition for searching and prints warning) or a user explicitly specifying that this particular condition is for searching. The first approach may lead to not optimal performance/cost if user doesn't notice the warning. The second one makes Dynamoid API more complex (e.g. a new method #where_keys could be added) and less generic (because the change is caused of a nuance/limitation of a low level Query operation) and doesn't prevent multiple #where_keys calls so it might be unclear which condition to use for searching.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants