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

ApplyViewToElementsFunction corrupts matchedVertex #3052

Open
t92549 opened this issue Nov 1, 2023 · 2 comments · May be fixed by #3057
Open

ApplyViewToElementsFunction corrupts matchedVertex #3052

t92549 opened this issue Nov 1, 2023 · 2 comments · May be fixed by #3057
Labels
bug Confirmed or suspected bug
Milestone
Medium Term Backlog

Comments

@t92549
Copy link
Contributor

t92549 commented Nov 1, 2023 •
edited

Describe the bug
The ApplyViewToElementsFunction merge function, which is the default merge function for federated stores, will corrupt the results' matchedVertex fields.

To Reproduce
Steps to reproduce the behaviour:

  1. Create FederatedStore -> AccumuloStore
  2. AddElements: A --> B
  3. GetElements(input=B)
  4. Result: A --> B: incorrectly shows matchedVertex = SOURCE

Expected behaviour
The resulting Edge should have matchedVertex = DESTINATION.

Additional context
ApplyViewToElementsFunction creates an internal MapStore to apply the view to all the results. Clearly, this is not applying the matchedVertex field properly from the actual results.
The same query works fine with the ConcatenateMergeFunction.
This bug also breaks GetWalks, because it relies on the matchedVertex field.

Platform

  • Gaffer Version: [e.g. 2.0.0]
@t92549 t92549 added the bug Confirmed or suspected bug label Nov 1, 2023
GCHQDev404 added a commit that referenced this issue Nov 2, 2023
@GCHQDev404
gh-3052 Test for fixing corrupt MatchedVertex.
d715ee1
@GCHQDev404
Copy link
Contributor

GCHQDev404 commented Nov 2, 2023 •
edited

examine https://github.com/gchq/Gaffer/tree/gh-3052-FederatedStore-Function-corrupts-matched-vertex for the test required to pass to prove/fix this.

Gaffer/store-implementation/federated-store/src/test/java/uk/gov/gchq/gaffer/federatedstore/util/ApplyViewToElementsFunctionTest.java

Lines 188 to 232 in 36063e2

void shouldPreserveMatchedVertex() throws Exception {
//given
final AccumuloStore accumuloStore = getTestStore("shouldPreserveMatchedVertex");
accumuloStore.execute(new AddElements.Builder()
.input(new Edge.Builder()
.source(SOURCE_BASIC)
.dest(DEST_BASIC)
.directed(true)
.group(GROUP_BASIC_EDGE)
.property(PROPERTY_1, FederatedStoreTestUtil.VALUE_PROPERTY1)
.build()).build(), contextBlankUser());
final View view = new View.Builder().edge(GROUP_BASIC_EDGE).build();
AccumuloElementsRetriever[] retrievers = getRetrievers(accumuloStore, new GetElements.Builder().input(DEST_BASIC).inOutType(INCOMING).view(view).build());
final ApplyViewToElementsFunction function = new ApplyViewToElementsFunction().createFunctionWithContext(
makeContext(view, SCHEMA.clone()));
//when
Iterable<Object> iterable = null;
for (AccumuloElementsRetriever elements : retrievers) {
iterable = function.apply(elements, iterable);
}
//then
final Edge edge5 = new Edge.Builder()
.source(SOURCE_BASIC)
.dest(DEST_BASIC)
.group(GROUP_BASIC_EDGE)
.directed(true)
//With aggregated property value of 5
.property(PROPERTY_1, 5)
.build();
assertThat(iterable)
.asInstanceOf(InstanceOfAssertFactories.iterable(Element.class))
.containsExactly(edge5);
//then because edge.equals ignores VertexMatched
assertThat(iterable.iterator().next())
.asInstanceOf(InstanceOfAssertFactories.type(Edge.class))
.extracting(Edge::getMatchedVertex)
.isEqualTo(EdgeId.MatchedVertex.DESTINATION);
}

@github-actions github-actions bot linked a pull request Nov 2, 2023 that will close this issue
Draft
GCHQDev404 added a commit that referenced this issue Nov 2, 2023
@GCHQDev404
gh-3052 One line fix, bug in MapStore.
b73cf69
GCHQDev404 added a commit that referenced this issue Nov 2, 2023
@GCHQDev404
gh-3052 typo
7ad514b
@t92549 t92549 mentioned this issue Nov 2, 2023
Open
GCHQDev404 added a commit that referenced this issue Nov 2, 2023
@GCHQDev404
Revert "gh-3052 One line fix, bug in MapStore."
36063e2 
This reverts commit b73cf69.
GCHQDev404 added a commit that referenced this issue Feb 27, 2024
@GCHQDev404
Merge branch 'develop' into gh-3052-FederatedStore-Function-corrupts-…
a5d6177 
…matched-vertex
@GCHQDev404
Copy link
Contributor

#3057

Test to be fixed to resolve this ticket.

@GCHQDeveloper314 GCHQDeveloper314 added this to the Medium Term Backlog milestone Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Confirmed or suspected bug
Projects
None yet
Milestone
Medium Term Backlog
Development

Successfully merging a pull request may close this issue.

gh-3052 Test for fixing corrupt MatchedVertex. gchq/Gaffer
3 participants
@GCHQDev404 @t92549 @GCHQDeveloper314