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

feat: add binary data type support to bwrite command #170

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

tmyoda
Copy link
Contributor

@tmyoda tmyoda commented Sep 15, 2023

Issue #, if available:
#168

Description of changes:
This PR adds support for Binary data types (B, BS) tobwrite command.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@tmyoda tmyoda marked this pull request as ready for review October 3, 2023 22:17
@StoneDot StoneDot self-requested a review October 8, 2023 02:36
Copy link
Contributor

@StoneDot StoneDot left a 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.

@tmyoda
Copy link
Contributor Author

tmyoda commented Oct 10, 2023

Thank you for the review!
I've added an integration test to verify that the current implementation works correctly without the need for base64 encoding.

Please let me know if there's anything else you'd like me to address.

@tmyoda tmyoda requested a review from StoneDot October 10, 2023 00:54
Copy link
Contributor

@StoneDot StoneDot left a 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?

Cargo.toml Outdated Show resolved Hide resolved
tests/bwrite.rs Outdated Show resolved Hide resolved
tests/bwrite.rs Outdated Show resolved Hide resolved
src/batch.rs Outdated Show resolved Hide resolved
src/batch.rs Outdated Show resolved Hide resolved
@StoneDot
Copy link
Contributor

Based on my testing, I confirmed that dy get -o raw is correct.

❯ 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
        }
    }
}

@tmyoda
Copy link
Contributor Author

tmyoda commented Oct 11, 2023

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 rusoto_dynamodb::DynamoDbClient's batch_write_item encodes the input to base64.

https://rusoto.github.io/rusoto/src/rusoto_dynamodb/generated.rs.html#4714-4740

As you suggested, I will decode the input data before calling batch_write_item.

Copy link
Contributor

@StoneDot StoneDot left a 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.

src/batch.rs Outdated Show resolved Hide resolved
src/batch.rs Outdated Show resolved Hide resolved
src/batch.rs Outdated Show resolved Hide resolved
@tmyoda tmyoda force-pushed the bwrite-binary branch 2 times, most recently from 29ac72b to df1a8d0 Compare October 23, 2023 21:40
@tmyoda
Copy link
Contributor Author

tmyoda commented Oct 23, 2023

Thank you so much for your thorough review! I have implemented the changes and left a comment.

@tmyoda tmyoda requested a review from StoneDot October 23, 2023 22:02
Copy link
Contributor

@StoneDot StoneDot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

@StoneDot StoneDot merged commit 878b1d4 into awslabs:main Oct 26, 2023
2 checks passed
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.

2 participants