-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat: add binary data type support to bwrite
command
#170
Conversation
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.
Thank you for implementing the support of the binary data type in the bwrite
command. This is a great addition!
I had doubts about whether base64 decoding was required to put the correct items. After conducting my investigation, it seems that the current implementation is indeed correct, and there's no need for base64 decoding. If the tests validate this, that's excellent. Would it be possible for you to include a test case to verify this?
Please let me know if you intend to address this in a separate pull request.
Thank you for the review! Please let me know if there's anything else you'd like me to address. |
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.
Based on the test codes, the binary data seems double-encoded by base64
. My check was something wrong. Could you please fix this?
Based on my testing, I confirmed that ❯ cargo run bwrite --input tests/resources/test_batch_write.json
❯ cargo run get ichi -o raw
Finished dev [unoptimized + debuginfo] target(s) in 0.22s
Running `target/debug/dy get ichi -o raw`
{
"BinarySet": {
"BS": [
"VTI1dmQzaz0=",
"VTNWdWJuaz0=",
"VW1GcGJuaz0="
]
},
"Nothing": {
"NULL": true
},
"PageCount": {
"NS": [
"-19",
"3.14",
"7.5",
"42.2"
]
},
"ISBN": {
"S": "111-1111111111"
},
"Dimensions": {
"SS": [
"Giraffe",
"Hippo",
"Zebra"
]
},
"Binary": {
"B": "ZEdocGN5QjBaWGgwSUdseklHSmhjMlUyTkMxbGJtTnZaR1Zr"
},
"Details": {
"M": {
"Age": {
"N": "35"
},
"Misc": {
"M": {
"dream": {
"L": [
{
"N": "35"
},
{
"NULL": true
}
]
},
"hope": {
"BOOL": true
}
}
},
"Name": {
"S": "Joe"
}
}
},
"InPublication": {
"BOOL": false
},
"Price": {
"N": "2"
},
"pk": {
"S": "ichi"
},
"Authors": {
"L": [
{
"S": "Author1"
},
{
"S": "Author2"
},
{
"N": "42"
}
]
}
}
❯ aws dynamodb get-item --endpoint http://localhost:8000 --region local --table-name __TABLE_NAME__ --key '{"pk":{"S":"ichi"}}'
{
"Item": {
"InPublication": {
"BOOL": false
},
"Details": {
"M": {
"Misc": {
"M": {
"hope": {
"BOOL": true
},
"dream": {
"L": [
{
"N": "35"
},
{
"NULL": true
}
]
}
}
},
"Age": {
"N": "35"
},
"Name": {
"S": "Joe"
}
}
},
"BinarySet": {
"BS": [
"VTI1dmQzaz0=",
"VTNWdWJuaz0=",
"VW1GcGJuaz0="
]
},
"PageCount": {
"NS": [
"-19",
"3.14",
"7.5",
"42.2"
]
},
"ISBN": {
"S": "111-1111111111"
},
"Price": {
"N": "2"
},
"Authors": {
"L": [
{
"S": "Author1"
},
{
"S": "Author2"
},
{
"N": "42"
}
]
},
"Dimensions": {
"SS": [
"Giraffe",
"Hippo",
"Zebra"
]
},
"pk": {
"S": "ichi"
},
"Binary": {
"B": "ZEdocGN5QjBaWGgwSUdseklHSmhjMlUyTkMxbGJtTnZaR1Zr"
},
"Nothing": {
"NULL": true
}
}
} |
Thank you again for your review. I apologize for the misunderstanding. I initially believed that your request was to check if the input is always encoded once. Based on my investigation, it appears that https://rusoto.github.io/rusoto/src/rusoto_dynamodb/generated.rs.html#4714-4740 As you suggested, I will decode the input data before calling |
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.
Thank you for your modification.
I apologize for the repeated request, but could you check my comments? I think that there is some room for improvement to this pull request to become a more great one.
29ac72b
to
df1a8d0
Compare
Thank you so much for your thorough review! I have implemented the changes and left a comment. |
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.
LGTM. Thank you!
Issue #, if available:
#168
Description of changes:
This PR adds support for Binary data types (B, BS) to
bwrite
command.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.