Skip to content

Conversation

@darrellwarde
Copy link
Contributor

Description

Note

Please provide a description of the work completed in this PR below

Aggregation fields were being created for relationship fields to single nodes, which serves no functional utility. These will be removed in 4.0.0.

Note: I have no idea why one of the TCK tests changed here!

Complexity

Note

Please provide an estimated complexity of this PR of either Low, Medium or High

Complexity: Low

Issue

Note

Please link to the GitHub issue(s) in which the proposal for this work was discussed

To link to multiple issues, use full syntax for each, for example Closes #1, closes #2, closes #3

Closes #2969

Checklist

The following requirements should have been met (depending on the changes in the branch):

  • Documentation has been updated
  • TCK tests have been updated
  • Integration tests have been updated
  • Example applications have been updated
  • New files have copyright header
  • CLA (https://neo4j.com/developer/cla/) has been signed

@changeset-bot
Copy link

changeset-bot bot commented Mar 10, 2023

🦋 Changeset detected

Latest commit: 47acc57

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@neo4j/graphql Major
@neo4j/graphql-toolbox Patch
@neo4j/graphql-ogm Major
neo-place Patch
neo-push-server Patch
apollo-suscriptions-server Patch
yoga-subscriptions-server Patch
@neo4j/graphql-plugin-subscriptions-amqp Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@neo4j-team-graphql
Copy link
Collaborator

neo4j-team-graphql commented Mar 10, 2023

Performance Report

No Performance Changes

Show Full Table
name dbHits old dbHits time (ms) old time (ms) maxRows
aggregations.TopLevelAggregate 3403 3403 46 70 1134
aggregations.NestedAggregation 14551 14551 106 137 2174
aggregations.AggregationWithWhere 11979 11979 70 160 2174
aggregations.AggregationWhereWithinNestedRelationships 22116987 22116987 4380 4143 2008534
aggregations.AggregationWhereWithinNestedConnections 22116987 22116987 3595 3646 2008534
aggregations.NestedCountFromMovieToActors 9734 9734 120 72 2174
aggregations.NestedCountFromActorsToMovie 9937 9937 128 176 2174
aggregations.DeeplyNestedCount 13070329 13070329 5889 6111 2008534
batch-create.BatchCreate 4200 4200 253 260 600
connect.createAndConnect 14424 14424 213 261 3003
connections.Connection 14082 14082 115 109 2174
connections.NestedConnection 41464 41468 147 189 4516
connections.ConnectionWithSort 2282 2282 79 100 1040
connections.ConnectionWithSortAndCypher 15323 15323 108 149 2174
create.SimpleMutation 7 7 66 96 1
cypher-directive.TopLevelMutationDirective 1135 1135 34 37 1134
delete.SimpleDelete 19401 19401 319 339 1040
delete.NestedDeleteInUpdate 18540 18540 239 383 1760
2925.SingleRelationshipFilter 6468 6468 144 57 1040
query.SimpleQuery 15160 15160 136 65 2174
query.QueryWhere 9712 9705 46 47 2167
query.SimpleQueryWithNestedWhere 9890 9883 62 79 2167
query.Nested 10092037 10092037 11983 11832 2008534
query.NestedWithFilter 40401 40401 222 175 4511
query.OrFilterOnRelationships 42913 42476 251 312 1871
query.OrFilterOnRelationshipsAndNested 37054 36454 260 308 1871
query.QueryWithNestedIn 14005 13229 70 73 1421
query.NestedConnectionWhere 9834 9834 76 79 2174
query.DeeplyNestedConnectionWhere 9836 9959 135 159 2174
query.DeeplyNestedWithRelationshipFilters 6343 6343 159 212 1134
query.NestedWithRelationshipSingleFilters 3024400 3024400 807 792 1003942
query.Fulltext 96 96 44 47 16
query.FulltextWithNestedQuery 587 587 61 66 84
sorting.SortMultipleTypes 2513 2513 83 89 1040
sorting.SortMultipleTypesWithCypherWithCypher 14520 14501 195 274 2174
sorting.SortOnNestedFields 14082 14082 139 124 2174
sorting.SortDeeplyNestedFields 42196 42196 174 237 4516
sorting.SortWithTopLevelCypher 15160 15160 76 69 2174
unions.SimpleUnionQuery 321 321 87 86 35
unions.SimpleUnionQueryWithMissingFields 293 293 71 92 35
unions.NestedUnion 410637 410637 412 391 33033
unions.NestedUnionWithMissingFields 384611 384611 404 403 33033
update.NestedUpdate 2119 2119 93 88 1040

Old Schema Generation: 43.676s
Schema Generation: 43.201s
Old Subgraph Schema Generation: 1:02.191 (m:ss.mmm)
Subgraph Schema Generation: 57.091s

Copy link
Contributor

@Liam-Doodson Liam-Doodson left a comment

Choose a reason for hiding this comment

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

LGTM! That tck test is weird but the logic hasn't changed luckily. I guess the order cypher builder has assigned the variables changed for some reason!

@Liam-Doodson
Copy link
Contributor

The failing TCK tests seem to have the new rel type escaping. I assume that's come frome dev but might be worth looking into because did we not say that was going to be a breaking change?

Copy link
Contributor

@MacondoExpress MacondoExpress left a comment

Choose a reason for hiding this comment

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

The changes look good to me but, to be fair, I'm very confused why the two predicates are translated in a different order in the TCK test!
Btw the cypher-builder variables are different because the query is composed in a different order.

@darrellwarde
Copy link
Contributor Author

The failing TCK tests seem to have the new rel type escaping. I assume that's come frome dev but might be worth looking into because did we not say that was going to be a breaking change?

Well this is merging into 4.0.0, so not a huge problem, just a bit weird really!

@Liam-Doodson
Copy link
Contributor

The failing TCK tests seem to have the new rel type escaping. I assume that's come frome dev but might be worth looking into because did we not say that was going to be a breaking change?

Well this is merging into 4.0.0, so not a huge problem, just a bit weird really!

Ah yes ignore me!

@darrellwarde darrellwarde merged commit 0fb2592 into neo4j:4.0.0 Mar 13, 2023
@darrellwarde darrellwarde deleted the fix/single-field-aggregations branch March 13, 2023 10:37
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.

Aggregate filter is wrongly generated for non-List relationship

4 participants