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.

Sign up for GitHub

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

Jump to bottom

DDB Enhanced: Added support for projection expression #1756

Merged
merged 1 commit into from
Apr 6, 2020
Merged

DDB Enhanced: Added support for projection expression #1756

merged 1 commit into from
Apr 6, 2020

Conversation

musketyr
Copy link
Contributor

@musketyr musketyr commented Apr 1, 2020

Description

Added missing support for projection expression into QueryEnhancedRequest and ScanEnhancedRequest classes.

Motivation and Context

At the moment, there is no way how to obtain just a portion of the data from DynamoDB in OOP manner.

Testing

The tests have been updated to reflect the change.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed
  • A short description of the change has been added to the CHANGELOG
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@codecov-io
Copy link

codecov-io commented Apr 1, 2020 •
edited
Loading

Codecov Report

Merging #1756 into master will decrease coverage by 0.02%.
The diff coverage is 82.85%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1756      +/-   ##
============================================
- Coverage     76.30%   76.27%   -0.03%     
  Complexity      182      182              
============================================
  Files          1072     1072              
  Lines         32252    32308      +56     
  Branches       2520     2520              
============================================
+ Hits          24610    24644      +34     
- Misses         6401     6417      +16     
- Partials       1241     1247       +6
Flag Coverage Δ Complexity Δ
#unittests 76.27% <82.85%> (-0.03%) 182.00 <0.00> (ø)
Impacted Files Coverage Δ Complexity Δ
.../enhanced/dynamodb/model/QueryEnhancedRequest.java 61.64% <64.70%> (+0.24%) 0.00 <0.00> (ø)
...k/enhanced/dynamodb/model/ScanEnhancedRequest.java 61.40% <64.70%> (+0.42%) 0.00 <0.00> (ø)
...d/dynamodb/internal/operations/QueryOperation.java 100.00% <100.00%> (ø) 0.00 <0.00> (ø)
...ed/dynamodb/internal/operations/ScanOperation.java 100.00% <100.00%> (ø) 0.00 <0.00> (ø)
...mazon/awssdk/utils/async/DelegatingSubscriber.java 77.77% <0.00%> (-22.23%) 0.00% <0.00%> (ø%)
...k/core/pagination/async/ResponsesSubscription.java 68.00% <0.00%> (-16.00%) 0.00% <0.00%> (ø%)
...ttp/nio/netty/internal/nrs/HttpStreamsHandler.java 60.76% <0.00%> (-3.85%) 0.00% <0.00%> (ø%)
...signer/internal/AwsChunkedEncodingInputStream.java 46.62% <0.00%> (-0.68%) 0.00% <0.00%> (ø%)
...tials/WebIdentityTokenFileCredentialsProvider.java 37.20% <0.00%> (-0.57%) 0.00% <0.00%> (ø%)
...ssdk/auth/credentials/HttpCredentialsProvider.java 69.01% <0.00%> (-0.44%) 0.00% <0.00%> (ø%)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c283043...92fa684. Read the comment docs.

@musketyr musketyr changed the title [DynamoDB Enhanced] Added support for projection expression DDB Enhanced: Added support for projection expression Apr 1, 2020
@musketyr
Copy link
Contributor Author

musketyr commented Apr 1, 2020

I'm not sure how can I fix the Quality Gate - the duplication and missing coverages origins in hashCode and equals.

bmaizels
bmaizels reviewed Apr 1, 2020
View reviewed changes
Copy link
Contributor

@bmaizels bmaizels left a comment

Choose a reason for hiding this comment

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

  1. Please add unit-tests for new behavior in ScanOperation and QueryOperation.
  2. Please add functional tests to prove this works correctly with Local DynamoDB (see existing functional tests in functionaltests/ folder under tests)

@bmaizels
Copy link
Contributor

bmaizels commented Apr 1, 2020

I'm not sure how can I fix the Quality Gate - the duplication and missing coverages origins in hashCode and equals.

Let's see what happens when we add the unit tests and functional tests that I mentioned in the review, might fix it, if not we can take another look then.

@bmaizels
Copy link
Contributor

bmaizels commented Apr 1, 2020

@musketyr This is a legit missing feature that we planned to follow up with in the future. Thanks for taking it on, we welcome these kinds of contributions.

@musketyr
Copy link
Contributor Author

musketyr commented Apr 1, 2020

Thanks @bmaizels for your feedback, I will change the method signature to attributesToProject and will add the tests you have mentioned.

@musketyr
Copy link
Contributor Author

musketyr commented Apr 2, 2020

hi, @bmaizels. I've added the required tests as you wanted. regarding the duplications - it would make sense and make the life of the toolsmiths easier when there is a common interface/ancestor to Scan* and Query* classes but it goes beyond this PR.

bmaizels
bmaizels reviewed Apr 2, 2020
View reviewed changes
bmaizels
bmaizels reviewed Apr 3, 2020
View reviewed changes
Copy link
Contributor

@bmaizels bmaizels left a comment

Choose a reason for hiding this comment

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

This is looking great. Just a few outstanding minor things to address. I'll try and resolve all the other conversations.

@bmaizels
Copy link
Contributor

bmaizels commented Apr 3, 2020

Ok I think we're in great shape. Just a few final procedural things. The first you can probably help me with: can we squash this into a single commit with a single concise descriptive commit message for the change? You'll need to squash it first in your local repo then force push to your branch.

@bmaizels
Copy link
Contributor

bmaizels commented Apr 3, 2020

Ok I think we're in great shape. Just a few final procedural things. The first you can probably help me with: can we squash this into a single commit with a single concise descriptive commit message for the change? You'll need to squash it first in your local repo then force push to your branch.

Actually let's wait and make sure checks pass first so you only have to do this once.

@musketyr
Copy link
Contributor Author

musketyr commented Apr 3, 2020

I can try so but in just wonder, isn't it the same as using "Squash and merge" option here on GitHub?

@bmaizels
Copy link
Contributor

bmaizels commented Apr 3, 2020

I can try so but in just wonder, isn't it the same as using "Squash and merge" option here on GitHub?

We disabled that for this repo unfortunately.

@bmaizels
Copy link
Contributor

bmaizels commented Apr 3, 2020

Don't worry about the quality gate I've examined the warnings and they are false positives as far as I'm concerned, so go ahead and squash whenever you are ready.

@musketyr musketyr force-pushed the feature/enhanced-dynamodb-projection-expression branch from 78f17be to 92fa684 Compare April 3, 2020 17:43
@musketyr
Copy link
Contributor Author

musketyr commented Apr 3, 2020

@bmaizels done, luckily it was a quite trivial thing to do.

@bmaizels
Copy link
Contributor

bmaizels commented Apr 3, 2020 •
edited
Loading

@musketyr Hey, the Changelog needs some conflicts fixing because a new release went out today. If you can fix that within the next few hours I should be able to merge this and it will go out in the next release. Thanks!

@musketyr
DDB Enhanced: Added support for projection expression
498040e 
added new property attributesToProject to QueryEnhancedRequest and ScanEnhancedRequest which enables to fetch
only partial results to save the bandwidth of the queries.
@musketyr musketyr force-pushed the feature/enhanced-dynamodb-projection-expression branch from 92fa684 to 498040e Compare April 4, 2020 04:00
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 4, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 3 Code Smells

80.4% 80.4% Coverage
39.6% 39.6% Duplication

@bmaizels bmaizels merged commit 8b85f34 into aws:master Apr 6, 2020
@bmaizels
Copy link
Contributor

bmaizels commented Apr 6, 2020

Merged. Thank you very much for your contribution!

@musketyr
Copy link
Contributor Author

musketyr commented Apr 6, 2020 •
edited
Loading

Thank you as well!

@metinkilic
Copy link

metinkilic commented Jul 17, 2020 •
edited
Loading

Hey @bmaizels @musketyr
There is a bug on this PR and I mentioned on the 8b85f34?diff=unified

This was referenced Sep 10, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmaizels bmaizels bmaizels approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@musketyr @codecov-io @bmaizels @metinkilic