Remove run first column #2769

angrykoala · 2023-01-23T16:45:53Z

Description

Removes runFirstColumn from the @cypher directive, forcing the directive to be used with the columnName option.

Updates all tests to comply with this new requirement

Complexity

Complexity: Medium

changeset-bot · 2023-01-23T16:45:57Z

🦋 Changeset detected

Latest commit: f2bd09c

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

CLAassistant · 2023-01-23T16:45:59Z

All committers have signed the CLA.

neo4j-team-graphql · 2023-01-23T16:54:57Z

Performance Report

name	dbHits	old dbHits	time (ms)	old time (ms)	maxRows
🟥 connections.ConnectionWithSortAndCypher	14278	2277	116	445	2174
🟥 cypher-directive.TopLevelCypherDirective	127994	99525	388	865	10168
🟥 sorting.SortMultipleTypesWithCypherWithCypher	13424	1439	127	312	2174
🟥 sorting.SortWithTopLevelCypher	14120	2119	58	211	2174

🟥 - Performance worsened (dbHits)
🟩 - Performance improved (dbHits)
🟦 - New test

Show Full Table

name	dbHits	old dbHits	time (ms)	old time (ms)	maxRows
aggregations.TopLevelAggregate	3403	3403	44	52	1134
aggregations.NestedAggregation	13514	13514	103	126	2174
aggregations.AggregationWithWhere	10942	10942	87	124	2174
aggregations.NestedCountFromMovieToActors	8694	8694	81	105	2174
aggregations.NestedCountFromActorsToMovie	8900	8900	81	126	2174
aggregations.DeeplyNestedCount	10054408	10054408	5157	5394	2008534
batch-create.BatchCreate	3600	3600	163	126	600
connect.createAndConnect	12419	12419	194	268	3003
connections.Connection	13042	13042	232	150	2174
connections.NestedConnection	33883	33883	136	171	4516
connections.ConnectionWithSort	2277	2277	71	84	1040
🟥 connections.ConnectionWithSortAndCypher	14278	2277	116	445	2174
create.SimpleMutation	6	6	32	51	1
🟥 cypher-directive.TopLevelCypherDirective	127994	99525	388	865	10168
delete.SimpleDelete	18361	18361	275	386	1040
delete.NestedDeleteInUpdate	16779	16779	187	158	2040
query.SimpleQuery	14120	14120	180	123	2174
query.QueryWhere	12971	12993	40	59	2161
query.SimpleQueryWithNestedWhere	13142	13164	70	87	2161
query.Nested	10084290	10084290	10738	11263	2008534
query.NestedWithFilter	38198	38198	144	138	4511
query.OrFilterOnRelationships	52737	50559	202	294	2139
query.OrFilterOnRelationshipsAndNested	40319	37664	240	394	2139
query.QueryWithNestedIn	14436	15315	70	97	1157
query.NestedConnectionWhere	13182	13182	64	70	2174
query.DeeplyNestedConnectionWhere	13186	13342	88	95	2174
query.DeeplyNestedWithRelationshipFilters	6913	6913	153	172	1134
query.DeeplyNestedWithRelationshipSingleFilters	15438	15438	102	120	2174
query.Fulltext	96	96	44	39	16
query.FulltextWithNestedQuery	571	571	49	55	84
sorting.SortMultipleTypes	2493	2493	82	81	1040
🟥 sorting.SortMultipleTypesWithCypherWithCypher	13424	1439	127	312	2174
sorting.SortOnNestedFields	13042	13042	106	102	2174
sorting.SortDeeplyNestedFields	38965	38965	190	227	4516
🟥 sorting.SortWithTopLevelCypher	14120	2119	58	211	2174
unions.SimpleUnionQuery	321	321	66	92	35
unions.SimpleUnionQueryWithMissingFields	293	293	64	84	35
unions.NestedUnion	398553	398553	341	364	33033
unions.NestedUnionWithMissingFields	372527	372527	288	341	33033
update.NestedUpdate	2117	2117	50	70	1040

Old Schema Generation: 35.521s
Schema Generation: 35.888s

Liam-Doodson · 2023-01-24T16:47:13Z

I've not had a chance to properly look over this yet but a general observation is that I think this could do with some updates to the documentation for the @cypher directive. Currently, most of the examples there do not use columnName.

The 4.0.0 migration guide also probably needs an update to make clear that at the point this is released, columnName will be required

angrykoala · 2023-01-25T09:17:46Z

Yeah, these will be added as a separate PR to reduce the size of these for review

Liam-Doodson

This is looking good, especially the performance improvement! Just a couple of typos in the updated tests and some edge cases that I'm not clear on the expected behaviour (mainly because I didn't see the first stage of this) and could possibly benefit from tests

Liam-Doodson · 2023-01-24T16:40:23Z

packages/graphql/src/constants.ts

-    "apoc.cypher.runFirstColumnMany",
-    "apoc.date.convertFormat",
-];
+export const REQUIRED_APOC_FUNCTIONS = ["apoc.util.validatePredicate", "apoc.date.convertFormat"];


Liam-Doodson · 2023-01-24T16:53:39Z

packages/graphql/tests/integration/directives/cypher/cypher.int.test.ts

@@ -79,7 +79,8 @@ describe("cypher", () => {
                                statement: """
                                MATCH (m:${Movie} {title: $title})
                                RETURN m
-                                """
+                                """,
+                                columnName: "m"


I wouldn't say these are blockers but it'd be interesting to see what the expected behaviour/error is if columnName is not provided now.
It'd also be interesting to see the behaviour if the wrong column name is provided.

Are there any tests that show either of these cases?

columnName is now be a mandatory field in the directive definition, so it will literally fail type definition validation if it isn't passed in.

Liam-Doodson · 2023-01-25T11:48:17Z

packages/graphql/src/schema/validation/validate-document.test.ts

                    shippingCost: Float
                        @cypher(
-                            statement: "MATCH (this)-[:SHIPS_TO]->(a:Address) RETURN round(0.01 * distance(a.location, Point({latitude: 40.7128, longitude: -74.0060})) / 1000, 2)"
+                            statement: "MATCH (this)-[:SHIPS_TO]->(a:Address) RETURN round(0.01 * distance(a.location, Point({latitude: 40.7128, longitude: -74.0060})) / 1000, 2) as result"
+                            columnName: "resul"


Suggested change

columnName: "resul"

columnName: "result"

Liam-Doodson · 2023-01-25T11:49:20Z

packages/graphql/src/schema/validation/validate-document.test.ts

@@ -402,6 +411,7 @@ describe("validateDocument", () => {
                    currentWeather: Weather
                        @cypher(
                            statement: "CALL apoc.load.json('https://www.7timer.info/bin/civil.php?lon=' + this.location.longitude + '&lat=' + this.location.latitude + '&ac=0&unit=metric&output=json&tzshift=0') YIELD value WITH value.dataseries[0] as weather RETURN {temperature: weather.temp2m, windSpeed: weather.wind10m.speed, windDirection: weather.wind10m.direction, precipitation: weather.prec_type, summary: weather.weather} AS conditions"
+                            columnName: "condition"


Suggested change

columnName: "condition"

columnName: "conditions"

Liam-Doodson · 2023-01-25T11:50:05Z

packages/graphql/src/schema/validation/validate-document.test.ts

@@ -435,6 +445,7 @@ describe("validateDocument", () => {
                            ORDER BY jaccard DESC
                            RETURN b LIMIT 1
                            """
+                            columnName: "jaccard"


Suggested change

columnName: "jaccard"

columnName: "b"

good catch!

Liam-Doodson · 2023-01-25T11:51:29Z

packages/graphql/src/translate/translate-top-level-cypher.ts

-            });
-            cypherStrs.push(...legacyCypherStatement);
-        }
+        const experimentalCypherStatement = createCypherDirectiveSubquery({


This probably shouldn't be called experimental anymore

Suggested change

const experimentalCypherStatement = createCypherDirectiveSubquery({

const cypherStatement = createCypherDirectiveSubquery({

Liam-Doodson · 2023-01-25T11:51:54Z

packages/graphql/src/translate/translate-top-level-cypher.ts

+        const experimentalCypherStatement = createCypherDirectiveSubquery({
+            field,
+        });
+        cypherStrs.push(...experimentalCypherStatement);


Suggested change

cypherStrs.push(...experimentalCypherStatement);

cypherStrs.push(...cypherStatement);

Liam-Doodson · 2023-01-25T11:55:34Z

packages/graphql/tests/integration/directives/cypher/cypher.int.test.ts

@@ -79,7 +79,8 @@ describe("cypher", () => {
                                statement: """
                                MATCH (m:${Movie} {title: $title})
                                RETURN m
-                                """
+                                """,
+                                columnName: "m"
                            )


Also not a blocker but I'm not clear what should happen if a user tried to return multiple columns in their cypher statement. Are there any tests for this?

it would get ignored, as columnName is used in with

Liam-Doodson · 2023-01-25T12:59:38Z

packages/graphql/tests/tck/issues/1139.test.ts

@@ -1,115 +0,0 @@
-/*


Why has this been deleted? Is it not possible to use columnName in this test?

This test is no longer relevant, as this is only caused when a cypher query does not return anything, which is no longer supported

darrellwarde

The changes are good! You are going to have to remind me, why is it that we decided against running EXPLAIN on the queries? I'm still really not 100% a fan of this approach, it feels somewhat like we're pinning our shortcomings onto our users.

darrellwarde · 2023-01-27T10:28:14Z

packages/graphql/src/schema/make-augmented-schema.test.ts

@@ -182,7 +182,7 @@ describe("makeAugmentedSchema", () => {
                }

                type Query {
-                    movies: [Movie!]! @cypher(statement: "")
+                    movies: [Movie!]! @cypher(statement: "RETURN 5 as a", columnName: "a")


The examples in this file upset me, I really wish there was a way we could have Cypher queries like this without the aliasing.

I agree, but none we could find :(

angrykoala · 2023-01-27T15:49:42Z

answering to you @darrellwarde

EXPLAIN has a lots of gotchas and pitfalls in its implementation, but most importantly, not all of the RETURN cases are covered, as explain does not really produce a consistent list of returned columns, with several cases requiring flaky custom parsing, and other cases simply not providing the variables.

I have the same concerns, and I feel that we could even keep providing a version without columnName using APOC, even if not recommended.

Another alternative could be trying to do some parsing if columnName is not provided. Even if this alternative does not work for all cases, columnName will always be available there to custom configure.

I'm happy to keep this PR open a bit longer if we want to keep this discussion going before merging

angrykoala added 3 commits January 23, 2023 13:51

Update @cypher directive on integration tests

78cfee4

Update unit tests

0ce4faf

Fix tck and schema tests

63818ae

angrykoala added 5 commits January 23, 2023 17:45

Remove runFirstColumn from cypher translation

4dc75d6

Merge branch '4.0.0' into remove-run-first-column

a75e607

Update examples

e36b235

Merge branch '4.0.0' into remove-run-first-column

b048db9

Update tests

5008aa7

angrykoala marked this pull request as ready for review January 24, 2023 14:10

Merge branch '4.0.0' into remove-run-first-column

d7368df

angrykoala requested review from Liam-Doodson and MacondoExpress January 24, 2023 16:34

Liam-Doodson reviewed Jan 25, 2023

View reviewed changes

Add changeset on columnName breaking change

e5b53a5

angrykoala mentioned this pull request Jan 26, 2023

Docs on @cypher columnName #2790

Merged

darrellwarde approved these changes Jan 27, 2023

View reviewed changes

Code review changes

f2bd09c

angrykoala merged commit 92bf74f into neo4j:4.0.0 Jan 30, 2023

angrykoala deleted the remove-run-first-column branch January 30, 2023 14:36

neo4j-team-graphql mentioned this pull request Jan 30, 2023

changesets for branch 4.0.0 (beta) #2774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove run first column #2769

Remove run first column #2769

angrykoala commented Jan 23, 2023

changeset-bot bot commented Jan 23, 2023 •

edited

Loading

CLAassistant commented Jan 23, 2023 •

edited

Loading

neo4j-team-graphql commented Jan 23, 2023 •

edited

Loading

Liam-Doodson commented Jan 24, 2023

angrykoala commented Jan 25, 2023

Liam-Doodson left a comment •

edited

Loading

Liam-Doodson Jan 24, 2023

Liam-Doodson Jan 24, 2023

darrellwarde Jan 27, 2023

Liam-Doodson Jan 25, 2023

Liam-Doodson Jan 25, 2023

Liam-Doodson Jan 25, 2023

angrykoala Jan 27, 2023

Liam-Doodson Jan 25, 2023

Liam-Doodson Jan 25, 2023

Liam-Doodson Jan 25, 2023

angrykoala Jan 27, 2023

Liam-Doodson Jan 25, 2023

angrykoala Jan 27, 2023

darrellwarde left a comment

darrellwarde Jan 27, 2023

angrykoala Jan 27, 2023

angrykoala commented Jan 27, 2023

	const experimentalCypherStatement = createCypherDirectiveSubquery({
	const cypherStatement = createCypherDirectiveSubquery({

	cypherStrs.push(...experimentalCypherStatement);
	cypherStrs.push(...cypherStatement);

Remove run first column #2769

Remove run first column #2769

Conversation

angrykoala commented Jan 23, 2023

Description

Complexity

changeset-bot bot commented Jan 23, 2023 • edited Loading

🦋 Changeset detected

CLAassistant commented Jan 23, 2023 • edited Loading

neo4j-team-graphql commented Jan 23, 2023 • edited Loading

Performance Report

Liam-Doodson commented Jan 24, 2023

angrykoala commented Jan 25, 2023

Liam-Doodson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darrellwarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angrykoala commented Jan 27, 2023

changeset-bot bot commented Jan 23, 2023 •

edited

Loading

CLAassistant commented Jan 23, 2023 •

edited

Loading

neo4j-team-graphql commented Jan 23, 2023 •

edited

Loading

Liam-Doodson left a comment •

edited

Loading