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

gh-2421: Change MapStore GetElements to handle Elements #2508

Merged
merged 3 commits into from
Sep 21, 2021
Merged

gh-2421: Change MapStore GetElements to handle Elements #2508

merged 3 commits into from
Sep 21, 2021

Conversation

t92549
Copy link
Contributor

@t92549 t92549 commented Sep 17, 2021 •
edited by github-actions bot
Loading

This should let the input to the MapStore's GetElements be an Element, as well as well as an ElementSeed.
Marked as draft as some test for this might be a good idea.

Related Issue

t92549
t92549 commented Sep 17, 2021
View reviewed changes
...implementation/map-store/src/main/java/uk/gov/gchq/gaffer/mapstore/impl/GetElementsUtil.java
@@ -99,12 +99,13 @@ private GetElementsUtil() {
} else {
relevantElements = new HashSet<>();

final EdgeId edgeId = (EdgeSeed) elementId;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the cast that was causing errors. Now we create a new EdgeSeed below, whether from an EdgeSeed or an Edge.

@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2021 •
edited
Loading

Codecov Report

Merging #2508 (7fd5c96) into develop (23b7371) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #2508   +/-   ##
==========================================
  Coverage      66.92%   66.92%           
  Complexity      2444     2444           
==========================================
  Files            973      973           
  Lines          31909    31909           
  Branches        3883     3883           
==========================================
  Hits           21355    21355           
  Misses          8886     8886           
  Partials        1668     1668
Impacted Files Coverage Δ
.../gchq/gaffer/mapstore/impl/AddElementsHandler.java 95.89% <100.00%> (ø)
...gov/gchq/gaffer/mapstore/impl/GetElementsUtil.java 91.95% <100.00%> (ø)
...java/uk/gov/gchq/gaffer/mapstore/impl/MapImpl.java 81.65% <100.00%> (ø)
...gchq/gaffer/parquetstore/query/QueryGenerator.java 84.55% <100.00%> (ø)

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 1ea1d6c...7fd5c96. Read the comment docs.

@t92549 t92549 marked this pull request as ready for review September 17, 2021 17:20
@t92549
Copy link
Contributor Author

t92549 commented Sep 17, 2021

This PR adds integration tests to ensure that all stores support GetElements' inputs being both ElementSeeds and Elements. This makes sense considering that input is of type ElementId, of which ElementSeed and Element are sub-classes of.
In order for these to pass, not only was MapStore fixed, but so was ParquetStore as it failed the tests.

@t92549 t92549 mentioned this pull request Sep 21, 2021
Closed
@p3430233 p3430233 merged commit 897253a into develop Sep 21, 2021
@p3430233 p3430233 deleted the gh-2421-mapstore-input-edge branch September 21, 2021 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p3430233 p3430233 p3430233 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.

Some stores don't allow Elements as GetElements input
3 participants
@t92549 @codecov-commenter @p3430233