Dynamoid / dynamoid

Ruby ORM for Amazon's DynamoDB.
MIT License
580 stars 195 forks source link

Migrate to expression attributes for Query and Scan #655

Closed andrykonchin closed 1 year ago

andrykonchin commented 1 year ago

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:

codecov[bot] commented 1 year ago

Codecov Report

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

:exclamation: 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%) :arrow_down:
lib/dynamoid/adapter.rb 100.00% <100.00%> (ø)
lib/dynamoid/adapter_plugin/aws_sdk_v3.rb 94.30% <100.00%> (-0.18%) :arrow_down:
...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.

github-actions[bot] commented 1 year ago

Code Coverage

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

Minimum allowed line rate is 90%

nbulaj commented 6 months ago

Hi @andrykonchin :wave:

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 commented 6 months ago

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.