Conversation
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
ysaito1001
left a comment
ysaito1001
left a comment
There was a problem hiding this comment.
Left minor questions, but bootstrapping looks good 🚀
|
|
||
| /// Converts this KeySpec to a HashMap suitable for DynamoDB API calls. | ||
| pub fn to_key_map(&self) -> HashMap<String, AttributeValue> { | ||
| let mut map = HashMap::new(); |
There was a problem hiding this comment.
Can we use with_capacity ?
| } | ||
| } | ||
|
|
||
| /// Helper trait for creating KeySpec from partition-key-only tables. |
There was a problem hiding this comment.
Nit: Helper function rather than helper trait?
| /// For converting complete structs to/from DynamoDB items, see [`ItemConverter`]. | ||
| pub trait AttributeValueConvert: Clone + Send + Sync + 'static { | ||
| /// Converts this value to a DynamoDB AttributeValue. | ||
| fn to_attribute_value(&self) -> Result<AttributeValue, ConversionError>; |
There was a problem hiding this comment.
Question: should there be into_attribute_value(self) -> Result<AttributeValue, ConversionError> like the Into trait? The implementers of this trait method in covert.rs internally clone incoming references anyway and AttributeValue is cloneable.
|
|
||
| [dependencies] | ||
| aws-sdk-dynamodb = "1" | ||
| aws-smithy-types = "1" |
There was a problem hiding this comment.
Do (or will) we use this dependency (i.e. anything other than re-exports from aws-sdk-dynamodb)?
landonxjames
left a comment
landonxjames
left a comment
There was a problem hiding this comment.
LGTM! Couple of questions, but nothing blocking
| @@ -0,0 +1,17 @@ | |||
| [package] | |||
| name = "aws-sdk-dynamodb-mapper" | |||
There was a problem hiding this comment.
Are we sold on dynamodb-mapper? I know thats what the Java library is called so maybe that is just the branding we go with. But I don't find it very descriptive. Is it an ORM? Is it more like serde-dynamo? I don't actually have a better name to suggest though so ¯_(ツ)_/¯
There was a problem hiding this comment.
It's open ended but likely a point to be decided by the SEP.
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| //! AttributeValueConvert implementations for standard Rust types. |
There was a problem hiding this comment.
Maybe worth having feature gated conversions for serde-dynamo types in here as well? Not necessary this early, but we probably should be thinking about how we will integrate with popular existing libraries.
| /// Represents a complete DynamoDB key specification with attribute names and values. | ||
| #[derive(Debug, Clone)] | ||
| pub struct KeySpec { | ||
| partition_keys: Vec<(String, AttributeValue)>, |
There was a problem hiding this comment.
We were already discussing the issue with too many allocations for DDB calls, wonder if this should be a slice just to not force more allocations? Probably a premature optimization at this point
There was a problem hiding this comment.
Yeah it affects quite a bit actually. Having a slice or even Cow here has a lot of fallout.
3 participants
Description
Bootstrap DDB mapper crates and add some of the core traits and types.
This is a POC implementation based on a working design.
Checklist
.changelogdirectory, specifying "client," "server," or both in theapplies_tokey..changelogdirectory, specifying "aws-sdk-rust" in theapplies_tokey.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.