-
Notifications
You must be signed in to change notification settings - Fork 19
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
PartiQL #256
PartiQL #256
Conversation
Thanks for your contribution @ChrixApp! I will try to review and test it over the weekend. |
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.
I don't think that PartiQL API in its current form should be present in high-level API. While direct mapping to object can be important, the API itself looks out of place. Ideally, it should follow the same pattern as other methods. So something like this:
context.PartiQL().WithStatement("statement").WithParameters(parameters).ExecuteAsync();
Maybe it even worth having a statement builder similarly to UpdateExpression builder.
I understand that it's a big chunk of work so I'd suggest focusing on low-level API in this PR and leave the high-level API for future PRs.
The low-level implementation looks good overall but there are some comments about input/output types and overall consistency. After these are addressed we can merge the low-level API.
src/EfficientDynamoDb/Operations/BatchExecuteStatement/BatchExecuteStatementRequest.cs
Outdated
Show resolved
Hide resolved
src/EfficientDynamoDb/Operations/BatchExecuteStatement/BatchStatementError.cs
Outdated
Show resolved
Hide resolved
src/EfficientDynamoDb/Operations/ExecuteStatement/ExecuteStatementRequest.cs
Outdated
Show resolved
Hide resolved
src/EfficientDynamoDb/Operations/ExecuteTransaction/ExecuteTransactionRequest.cs
Outdated
Show resolved
Hide resolved
I've made the necessary changes based on your feedback, @firenero |
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.
Thanks for addressing the feedback and sorry for the delay on my side. I was on vacation last week so next reviews will be much faster.
There are several more improvement points that I missed in the first review and it would be great to address them too.
src/EfficientDynamoDb/Operations/ExecuteStatement/ExecuteStatementResponse.cs
Outdated
Show resolved
Hide resolved
...EfficientDynamoDb/Internal/Operations/ExecuteStatement/ExecuteStatementRequestHttpContent.cs
Outdated
Show resolved
Hide resolved
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.
Thanks for making changes. ExecuteStatement
now works well. However, there are several issues in batch and transact responses parsing that I didn't notice in previous reviews.
...entDynamoDb/Internal/Operations/BatchExecuteStatement/BatchExecuteStatementResponseParser.cs
Outdated
Show resolved
Hide resolved
...EfficientDynamoDb/Internal/Operations/ExecuteTransaction/ExecuteTransactionResponseParser.cs
Outdated
Show resolved
Hide resolved
src/EfficientDynamoDb/DynamoDbContext/IDynamoDbLowLevelContext.cs
Outdated
Show resolved
Hide resolved
Hi @firenero , are there any additional change needed? |
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.
It looks good now. There are only a couple small inefficiencies in response parsing. We can merge the PR after they are fixed.
...entDynamoDb/Internal/Operations/BatchExecuteStatement/BatchExecuteStatementResponseParser.cs
Outdated
Show resolved
Hide resolved
...entDynamoDb/Internal/Operations/BatchExecuteStatement/BatchExecuteStatementResponseParser.cs
Outdated
Show resolved
Hide resolved
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.
It's perfect now! Huge thanks for submitting the PR and working with me on all these changes.
Thanks, @firenero! It was a pleasure working with you as well. We still have the high-level PartiQL implementation pending and I was wondering when you plan to make the next release so I can plan my projects and use low-level PartiQL. |
@ChrixApp can you use pre-release version? If yes, PartiQL is available in 0.9.17-alpha.0.1. I was thinking about completing my work for #251 to bundle both features into single release. It might take a week or two because I have limited availability now. But let me know if you absolutely can't use pre-release version and I'll release stable with only PartiQL change early next week. |
I can use the pre-release version 0.9.17-alpha.0.1, thanks @firenero |
PartiQL statements have to be passed as strings, and all parameters have to be passed as attribute values.
It provides a low-level implementation that returns a response containing Documents.
There is also a way to map the response directly into classes to skip the attribute allocations.
If anything needs to be added or modified, please give me feedback, and I'll make the necessary changes.