This repository has been archived by the owner on Sep 3, 2021. It is now read-only.
Improve order by performance #247
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I would also like to bring up my difficulties to try out a modified version of
I get the following error: It seems that running When I point the package How am I supposed to try out this library in a local project? What did work was to point the # in package.json of `neo4j-graphql-js`
{
"name": "neo4j-graphql-js",
"version": "2.6.0",
"description": "A GraphQL to Cypher query execution layer for Neo4j. ",
"main": "/home/robert/Development/human-connection/Human-Connection/backend/node_modules/neo4j-graphql-js/dist/index.js",
"scripts": {
"start": "nodemon ./example/apollo-server/movies.js --exec babel-node -e js",
"autogen": "nodemon ./example/autogenerated/autogen.js --exec babel-node -e js",
"start-middleware": "nodemon ./example/apollo-server/movies-middleware.js --exec babel-node -e js",
"start-typedefs": "nodemon ./example/apollo-server/movies-typedefs.js --exec babel-node -e js",
"build": "babel src --presets babel-preset-env --out-dir /home/robert/Development/human-connection/Human-Connection/backend/node_modules/neo4j-graphql-js/dist",
"build-with-sourcemaps": "babel src --presets babel-preset-env --out-dir /home/robert/Development/human-connection/Human-Connection/backend/node_modules/neo4j-graphql-js/dist --source-maps",
...How do you setup this library for local development? |
roschaefer
changed the title
|
For development, I use Building the library locally and importing locally I believe has also worked for me. The issue here is that For running all tests locally, run a local neo4j instance with this database loaded with username (as mentioned in #248 we need to improve / document this process better) |
|
@johnymontana thanks a lot for pointing out the necessary steps. I was able to import the database locally. Oddly, plenty of integration tests fail: Terminal outputrobert@e480 ~/D/neo4j-graphql-js> git status
HEAD detached at origin/master
nothing to commit, working tree clean
robert@e480 ~/D/neo4j-graphql-js> git rev-parse HEAD
af92564fd0207cc8d4caaefa1315247c0b0a782d
robert@e480 ~/D/neo4j-graphql-js> npm install
npm WARN lifecycle The node binary used for scripts is /home/robert/.asdf/shims/node but npm is using /home/robert/.asdf/installs/nodejs/10.15.1/bin/node itself. Use the `--scripts-prepend-node-path` option to include the path for the node binary npm was executed with.
> [email protected] install /home/robert/Development/neo4j-graphql-js/node_modules/husky
> node ./bin/install.js
husky
setting up Git hooks
done
> [email protected] postinstall /home/robert/Development/neo4j-graphql-js/node_modules/protobufjs
> node scripts/postinstall
> [email protected] postinstall /home/robert/Development/neo4j-graphql-js/node_modules/nodemon
> node bin/postinstall || exit 0
Love nodemon? You can now support the project via the open collective:
> https://opencollective.com/nodemon/donate
npm WARN prepublish-on-install As of npm@5, `prepublish` scripts are deprecated.
npm WARN prepublish-on-install Use `prepare` for build steps and `prepublishOnly` for upload-only.
npm WARN prepublish-on-install See the deprecation note in `npm help scripts` for more information.
> [email protected] prepublish /home/robert/Development/neo4j-graphql-js
> npm run build
npm WARN lifecycle The node binary used for scripts is /home/robert/.asdf/shims/node but npm is using /home/robert/.asdf/installs/nodejs/10.15.1/bin/node itself. Use the `--scripts-prepend-node-path` option to include the path for the node binary npm was executed with.
> [email protected] build /home/robert/Development/neo4j-graphql-js
> babel src --presets babel-preset-env --out-dir dist
src/augment.js -> dist/augment.js
src/auth.js -> dist/auth.js
src/index.js -> dist/index.js
src/inferSchema.js -> dist/inferSchema.js
src/neo4j-schema/Neo4jSchemaTree.js -> dist/neo4j-schema/Neo4jSchemaTree.js
src/neo4j-schema/entities.js -> dist/neo4j-schema/entities.js
src/neo4j-schema/graphQLMapper.js -> dist/neo4j-schema/graphQLMapper.js
src/neo4j-schema/types.js -> dist/neo4j-schema/types.js
src/selections.js -> dist/selections.js
src/translate.js -> dist/translate.js
src/utils.js -> dist/utils.js
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
added 1083 packages from 733 contributors and audited 11030 packages in 14.783s
found 3 vulnerabilities (2 low, 1 high)
run `npm audit fix` to fix them, or `npm audit` for details
robert@e480 ~/D/neo4j-graphql-js> npm run parse-tck
npm WARN lifecycle The node binary used for scripts is /home/robert/.asdf/shims/node but npm is using /home/robert/.asdf/installs/nodejs/10.15.1/bin/node itself. Use the `--scripts-prepend-node-path` option to include the path for the node binary npm was executed with.
> [email protected] parse-tck /home/robert/Development/neo4j-graphql-js
> babel-node test/tck/parseTck.js
robert@e480 ~/D/neo4j-graphql-js> npm run test-all
npm WARN lifecycle The node binary used for scripts is /home/robert/.asdf/shims/node but npm is using /home/robert/.asdf/installs/nodejs/10.15.1/bin/node itself. Use the `--scripts-prepend-node-path` option to include the path for the node binary npm was executed with.
> [email protected] test-all /home/robert/Development/neo4j-graphql-js
> nyc ava test/*.js --verbose
✔ configTest › Config - makeAugmentedSchema - no queries, no mutations (214ms)
✔ configTest › Config - augmentSchema - no queries, no mutations
✔ filterTest › Unnamed test on line 101 (219ms)
✔ configTest › Config - makeAugmentedSchema - enable queries, no mutations
✔ configTest › Config - augmentSchema - enable queries, no mutations
✔ augmentSchemaTest › Test augmented schema (222ms)
✔ filterTest › Unnamed test on line 115
✔ filterTest › Unnamed test on line 129
✔ filterTest › Unnamed test on line 143
✔ filterTest › Unnamed test on line 157
✔ filterTest › Unnamed test on line 171
✔ configTest › Config - makeAugmentedSchema - enable queries, enable mutations
✔ filterTest › Unnamed test on line 185
✔ filterTest › Unnamed test on line 199
✔ configTest › Config - augmentSchema - enable queries, enable mutations
✔ configTest › Config - makeAugmentedSchema - specify types to exclude for mutation
✔ filterTest › Unnamed test on line 213
✔ configTest › Config - augmentSchema - specify types to exclude for mutation
✔ filterTest › Unnamed test on line 227
✔ configTest › Config - makeAugmentedSchema - specify types to exclude for query
✔ filterTest › Unnamed test on line 241
✔ filterTest › Unnamed test on line 255
✔ filterTest › Unnamed test on line 269
✔ configTest › Config - augmentSchema - specify types to exclude for query
✔ configTest › Config - temporal - disable temporal schema augmentation
✔ filterTest › Unnamed test on line 291
✔ filterTest › Unnamed test on line 311
✔ filterTest › Unnamed test on line 325
✔ filterTest › Unnamed test on line 339
✔ filterTest › Unnamed test on line 353
✔ filterTest › Unnamed test on line 367
✔ filterTest › Unnamed test on line 381
✔ filterTest › Unnamed test on line 395
✔ filterTest › Unnamed test on line 409
✔ filterTest › Unnamed test on line 423
✔ filterTest › Unnamed test on line 437
✔ filterTest › Unnamed test on line 451
✔ filterTest › Unnamed test on line 465
✔ filterTest › Unnamed test on line 479
✔ filterTest › Unnamed test on line 497
✔ filterTest › Unnamed test on line 515
✔ filterTest › Unnamed test on line 533
✔ filterTest › Unnamed test on line 547
✔ filterTest › Unnamed test on line 561
✔ filterTest › Unnamed test on line 575
✔ filterTest › Unnamed test on line 589
✔ filterTest › Unnamed test on line 603
✔ filterTest › Unnamed test on line 617
✔ filterTest › Unnamed test on line 631
✔ filterTest › Unnamed test on line 645
✔ filterTest › Unnamed test on line 659
✔ filterTest › Unnamed test on line 673
✔ filterTest › Unnamed test on line 687
✔ filterTest › Unnamed test on line 701
✔ filterTest › Unnamed test on line 715
✔ filterTest › Unnamed test on line 729
✔ filterTest › Unnamed test on line 743
✔ filterTest › Unnamed test on line 757
✔ filterTest › Unnamed test on line 775
✔ filterTest › Unnamed test on line 789
✔ filterTest › Unnamed test on line 803
✔ filterTest › Unnamed test on line 817
✔ filterTest › Unnamed test on line 831
✔ filterTest › Unnamed test on line 845
✔ filterTest › Unnamed test on line 859
✔ filterTest › Unnamed test on line 873
✔ filterTest › Unnamed test on line 887
✔ filterTest › Unnamed test on line 901
✔ filterTest › Unnamed test on line 915
✔ filterTest › Unnamed test on line 940
✔ filterTest › Unnamed test on line 956
✔ filterTest › Unnamed test on line 972
✔ filterTest › Unnamed test on line 986
✔ filterTest › Unnamed test on line 1000
✔ filterTest › Unnamed test on line 1014
✔ filterTest › Unnamed test on line 1028
✔ filterTest › Unnamed test on line 1042
✔ cypherTest › Handle @cypher directive on Mutation type
✔ cypherTest › Create node mutation
✔ cypherTest › Update node mutation
✔ cypherTest › Delete node mutation
✔ middleware › Middleware fail on req.error (134ms)
✔ integration › Add relationship mutation (259ms)
✔ integration › Remove relationship mutation
✔ integration › Temporal - Create node with temporal property
✔ integration › Temporal - Create node with multiple temporal fields and input formats
✔ integration › Temporal - Create node with multiple temporal fields and input formats - with GraphQL variables
✖ integration › Temporal - Query node with temporal field
✔ integration › Temporal - create node with only a temporal property
✖ integration › Temporal - temporal query argument, components
✖ integration › Temporal - temporal query argument, formatted
✔ integration › Add relationship with temporal property
✖ integration › Query for temporal property on relationship
✔ integration › hello world
✖ integration › Create node mutation Error: GraphQL error: Node(32394) already exists with label `Movie` and property `movieId` = '12dd334d5zaaaa'
✔ integration › basic GraphQL query (545ms)
✔ integration › Delete node mutation (615ms)
✖ integration › Mutation with @cypher directive Error: GraphQL error: Node(32374) already exists with label `Genre` and property `name` = 'Wildlife Documentary'
✔ integration › GraphQL query with @cypher directive (660ms)
✔ integration › Handle @cypher directive on QueryType (687ms)
✔ integration › Update node mutation (691ms)
✔ integration › query relationship property data (716ms)
✔ integration › Basic filter (737ms)
✔ integration › Filter with AND (917ms)
✔ integration › Top level orderBy (942ms)
✔ integration › query using inine fragment (965ms)
✔ integration › Filter with OR (1.1s)
✔ integration › Filter in selection (1.2s)
✔ integration › Filter with nested AND and OR (1.3s)
✔ integration › Nested filter (1.4s)
✔ integration › Filter with GraphQL variable (1.5s)
✔ cypherTest › Add relationship mutation (3.1s)
✔ cypherTest › Add relationship mutation with GraphQL variables (3s)
✔ cypherTest › Add relationship mutation with relationship property (3s)
✔ cypherTest › Add reflexive relationship mutation with relationship property (2.9s)
✔ cypherTest › Remove relationship mutation (2.9s)
✔ cypherTest › Remove reflexive relationship mutation (2.8s)
✔ cypherTest › orderBy test - descending, top level - augmented schema (2.4s)
✔ cypherTest › query for relationship properties (2.3s)
✔ cypherTest › query reflexive relation nested in non-reflexive relation (2.3s)
✔ cypherTest › query non-reflexive relation nested in reflexive relation (2.3s)
✔ cypherTest › query relation type with argument (2.2s)
✔ cypherTest › query reflexive relation type with arguments (2.2s)
✔ cypherTest › query using inline fragment (2.1s)
✔ cypherTest › Create node with temporal properties (2s)
✔ cypherTest › Query node with temporal properties using temporal arguments (2s)
✔ cypherTest › Nested Query with temporal property arguments (2s)
✔ cypherTest › Update temporal and non-temporal properties on node using temporal property node selection (1.9s)
✔ cypherTest › Update temporal list property on node using temporal property node selection (1.9s)
✔ cypherTest › Delete node using temporal property node selection (1.8s)
✔ cypherTest › Add relationship mutation using temporal property node selection (1.8s)
✔ cypherTest › Remove relationship mutation using temporal property node selection (1.8s)
✔ cypherTest › Query relationship with temporal properties (1.7s)
✔ cypherTest › Add relationship mutation with temporal properties (1.6s)
✔ cypherTest › Add relationship mutation with list properties (1.5s)
✔ cypherTest › Add reflexive relationship mutation with temporal properties (1.4s)
✔ cypherTest › Remove relationship mutation for relation type field (1.3s)
✔ cypherTest › Query nested temporal properties on reflexive relationship using temporal arguments (1.2s)
✔ cypherTest › Query nested temporal properties on relationships using temporal arguments (1.2s)
✔ cypherTest › Query nested list properties on relationship (1.1s)
✔ cypherTest › UUID value generated if no id value provided (1s)
✔ cypherTest › Create node with list arguments (1s)
✔ cypherTest › simple Cypher query (5.4s)
✔ cypherTest › Simple skip limit (5.1s)
✔ cypherTest › Cypher projection skip limit (5s)
✔ cypherTest › Handle Query with name not aligning to type (4.9s)
✔ cypherTest › Query without arguments, non-null type (4.7s)
✔ cypherTest › Query single object (4.6s)
✔ cypherTest › Query single object relation (4.5s)
✔ cypherTest › Query single object and array of objects relations (4.4s)
✔ cypherTest › Deeply nested object query (4.3s)
✔ cypherTest › Handle meta field at beginning of selection set (4.1s)
✔ cypherTest › Handle meta field at end of selection set (4.1s)
✔ cypherTest › Handle meta field in middle of selection set (4s)
✔ cypherTest › Handle @cypher directive without any params for sub-query (4s)
✔ cypherTest › Pass @cypher directive default params to sub-query (3.9s)
✔ cypherTest › Pass @cypher directive params to sub-query (3.8s)
✔ cypherTest › Query for Neo4js internal _id (3.8s)
✔ cypherTest › Query for Neo4js internal _id and another param before _id (3.7s)
✔ cypherTest › Query for Neo4js internal _id and another param after _id (3.6s)
✔ cypherTest › Query for Neo4js internal _id by dedicated Query MovieBy_Id(_id: String!) (3.5s)
✔ cypherTest › Query for null value translates to 'IS NULL' WHERE clause (3.5s)
✔ cypherTest › Query for null value combined with internal ID and another param (3.4s)
✔ cypherTest › Cypher subquery filters (3.3s)
✔ cypherTest › Cypher subquery filters with paging (3.2s)
✔ cypherTest › Handle @cypher directive on Query Type (3.2s)
✔ cypherTest › Handle GraphQL variables in nested selection - first/offset (2.8s)
✔ cypherTest › Handle GraphQL variables in nest selection - @cypher param (not first/offset) (2.8s)
✔ cypherTest › Return internal node id for _id field (2.7s)
✔ cypherTest › Treat enum as a scalar (2.7s)
✔ cypherTest › Handle query fragment (2.6s)
✔ cypherTest › Handle multiple query fragments (2.6s)
✔ cypherTest › nested fragments (2.5s)
✔ cypherTest › fragments on relations (2.5s)
✔ cypherTest › nested fragments on relations (2.4s)
✔ cypherTest › Cypher array queries (972ms)
✔ cypherTest › Cypher array sub queries (929ms)
✔ cypherTest › Create node with non-null field (867ms)
✔ cypherTest › Query node with ignored field (805ms)
✔ cypherTest › Deeply nested orderBy (714ms)
✔ cypherTest › Query using enum orderBy (684ms)
✔ cypherTest › Query using temporal orderBy (658ms)
✔ cypherTest › Deeply nested query using temporal orderBy (615ms)
✔ cypherTest › Handle @cypher field with String payload using cypherParams (585ms)
✔ cypherTest › Handle nested @cypher fields that use cypherParams (562ms)
✔ cypherTest › Handle @cypher query using cypherParams with String payload (539ms)
✔ cypherTest › Handle @cypher query using cypherParams with Object payload (516ms)
✔ cypherTest › Handle @cypher query with Boolean payload (476ms)
✔ cypherTest › Handle @cypher query with Int payload (450ms)
✔ cypherTest › Handle @cypher query with Float payload (410ms)
✔ cypherTest › Handle @cypher query with String list payload (383ms)
✔ cypherTest › Handle @cypher query with Int list payload (363ms)
✔ cypherTest › Handle @cypher query with Temporal payload (340ms)
✔ cypherTest › Handle @cypher mutation using cypherParams with String payload (295ms)
✔ cypherTest › Handle @cypher mutation using cypherParams with Object payload (271ms)
✔ cypherTest › Handle @cypher mutation with String list payload (212ms)
✔ cypherTest › Handle @cypher mutation with Temporal payload (166ms)
✔ cypherTest › Handle nested @cypher fields using parameterized arguments and cypherParams (132ms)
✔ cypherTest › Handle @cypher mutation with input type argument (107ms)
✔ cypherTest › Handle @cypher query with parameterized input type argument
✔ cypherTest › Handle @cypher field on root query type with scalar payload, no args
✔ cypherTest › Handle @cypher field with parameterized value for field of input type argument
6 tests failed
integration › Temporal - Query node with temporal field
/home/robert/Development/neo4j-graphql-js/test/integration.js:877
876: .then(data => {
877: t.deepEqual(data.data, expected.data);
878: })
Difference:
{
Movie: [
Object { … },
- {
- __typename: 'Movie',
- date: {
- __typename: '_Neo4jDate',
- formatted: '2010-01-02',
- },
- dateTime: {
- __typename: '_Neo4jDateTime',
- day: 2,
- hour: 0,
- millisecond: 0,
- minute: 0,
- month: 1,
- nanosecond: 0,
- second: 0,
- timezone: 'Z',
- year: 2010,
- },
- localDateTime: {
- __typename: '_Neo4jLocalDateTime',
- day: 2,
- formatted: '2010-01-02T00:00:00',
- hour: 0,
- minute: 0,
- month: 1,
- second: 0,
- year: 2010,
- },
- title: 'Bob Loblaw 3',
- },
- {
- __typename: 'Movie',
- date: {
- __typename: '_Neo4jDate',
- formatted: '2010-01-02',
- },
- dateTime: {
- __typename: '_Neo4jDateTime',
- day: 2,
- hour: 0,
- millisecond: 0,
- minute: 0,
- month: 1,
- nanosecond: 0,
- second: 0,
- timezone: 'Z',
- year: 2010,
- },
- localDateTime: {
- __typename: '_Neo4jLocalDateTime',
- day: 2,
- formatted: '2010-01-02T00:00:00',
- hour: 0,
- minute: 0,
- month: 1,
- second: 0,
- year: 2010,
- },
- title: 'Bob Loblaw 3',
- },
],
}
integration › Temporal - temporal query argument, components
/home/robert/Development/neo4j-graphql-js/test/integration.js:949
948: .then(data => {
949: t.deepEqual(data.data, expected.data);
950: })
Difference:
{
OnlyDate: [
Object { … },
- {
- __typename: 'OnlyDate',
- date: {
- __typename: '_Neo4jDate',
- formatted: '2020-11-10',
- },
- },
- {
- __typename: 'OnlyDate',
- date: {
- __typename: '_Neo4jDate',
- formatted: '2020-11-10',
- },
- },
],
}
integration › Temporal - temporal query argument, formatted
/home/robert/Development/neo4j-graphql-js/test/integration.js:986
985: .then(data => {
986: t.deepEqual(data.data, expected.data);
987: })
Difference:
{
OnlyDate: [
Object { … },
- {
- __typename: 'OnlyDate',
- date: {
- __typename: '_Neo4jDate',
- formatted: '2020-11-10',
- },
- },
- {
- __typename: 'OnlyDate',
- date: {
- __typename: '_Neo4jDate',
- formatted: '2020-11-10',
- },
- },
],
}
integration › Query for temporal property on relationship
/home/robert/Development/neo4j-graphql-js/test/integration.js:1075
1074: .then(data => {
1075: t.deepEqual(data.data, expected.data);
1076: })
Difference:
{
Movie: [
{
__typename: 'Movie',
ratings: [
Object { … },
- {
- __typename: '_MovieRatings',
- date: {
- __typename: '_Neo4jDate',
- formatted: '2018-12-18',
- },
- rating: 5,
- },
- {
- __typename: '_MovieRatings',
- date: {
- __typename: '_Neo4jDate',
- formatted: '2018-12-18',
- },
- rating: 5,
- },
],
title: 'Fire',
},
],
}
integration › Create node mutation
/home/robert/Development/neo4j-graphql-js/test/integration.js:246
245: .catch(error => {
246: t.fail(error);
247: });
Error: GraphQL error: Node(32394) already exists with label `Movie` and property `movieId` = '12dd334d5zaaaa'
integration › Mutation with @cypher directive
/home/robert/Development/neo4j-graphql-js/test/integration.js:201
200: .catch(error => {
201: t.fail(error);
202: });
Error: GraphQL error: Node(32374) already exists with label `Genre` and property `name` = 'Wildlife Documentary'
---------------------|----------|----------|----------|----------|-------------------|
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s |
---------------------|----------|----------|----------|----------|-------------------|
All files | 86.58 | 77.94 | 73.87 | 86.72 | |
dist | 94.59 | 82.9 | 94.12 | 94.85 | |
augment.js | 96.63 | 87.64 | 95.83 | 97.04 |... 1171,1172,1173 |
auth.js | 82.76 | 64.15 | 85.71 | 81.82 |... 56,59,60,63,71 |
index.js | 65.65 | 39.34 | 53.85 | 66.15 |... 29,261,263,265 |
selections.js | 90.4 | 75.64 | 71.43 | 90.4 |... 40,142,143,287 |
translate.js | 99.32 | 90.29 | 100 | 99.31 |... 10,824,920,934 |
utils.js | 93.46 | 81.8 | 95 | 94.01 |... 1042,1047,1048 |
dist/neo4j-schema | 25.77 | 14.02 | 7.69 | 25.77 | |
Neo4jSchemaTree.js | 24.53 | 25 | 4.55 | 24.53 |... 46,247,250,251 |
entities.js | 41.56 | 20 | 13.79 | 41.56 |... 15,216,222,223 |
graphQLMapper.js | 17.59 | 10.81 | 4.17 | 17.59 |... 54,256,258,260 |
types.js | 20 | 8.82 | 14.29 | 20 |... 01,106,107,110 |
---------------------|----------|----------|----------|----------|-------------------|
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] test-all: `nyc ava test/*.js --verbose`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] test-all script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR! /home/robert/.npm/_logs/2019-05-21T21_55_25_875Z-debug.logIs this expected? |
|
@roschaefer all tests should be passing. Some of the integration tests modify the database so each test run needs to start with a fresh version of the database. |
roschaefer
force-pushed
the
239-improve_order_by_performance
branch
from
05f7018
to
04dfa08
Compare
At the moment, it's not quite clear to me how to run the tests locally. I will let the build server run the tests for now and I hope to see some tests failing that would give me some direction. @johnymontana @jexp @michaeldgraham I would be very glad if you could help me out on how to run tests locally and what test cases you would expect to see. This commit is still work-in-progress.
roschaefer
changed the title
|
@johnymontana @michaeldgraham can I get a code review? I cannot request it through Github. |
Codecov Report
@@ Coverage Diff @@
## master #247 +/- ##
=======================================
Coverage 94.47% 94.47%
=======================================
Files 4 4
Lines 326 326
=======================================
Hits 308 308
Misses 18 18Continue to review full report at Codecov.
|
|
So lonely: @johnymontana @michaeldgraham @jexp A code review or any feedback would be welcome, too! |
johnymontana
approved these changes
May 29, 2019
There was a problem hiding this comment.
Looks good to me. An additional test to cover the case where a @cypher directive field is requested in the selection set but not specified in the orderBy arg (and we still get the optimized ordering) would be nice, but not required for this PR as I think the basic functionality is covered here.
|
|
||
| const orderByStatments = orderByArray.map(orderByVar => { | ||
| const { orderBy, order } = splitOrderByArg(orderByVar); | ||
| const hasNoCypherDirective = _.isEmpty( |
There was a problem hiding this comment.
To be explicit - hasNoCypherDirective here applies to the orderBy field. We should still be able to use the optimized ordering if a @cypher field is requested in the selection set but not in the orderBy arg
|
Awesome - thanks @roschaefer! Sorry for the delay, we had a long holiday weekend in the US that pushed things back a bit. This should improve performance significantly for basic ordering cases. You say you are using this in production? Have you noticed a considerable performance improvement? Anything that is measurable? It would be nice to have some statistics for comparison, but we can at least backtest for comparison once we implement #240 And thanks for dealing with the janky test set up :-) |
|
@johnymontana great! Yes, we noticed a considerable performance improvement. There is a public holiday in Germany, too, and I am on my mobile. So I'll give you the exact stats later. But the difference was from ~30 down to 2 seconds on my laptop. Our small server on kubernetes just crashed before. I can send some more PRs to add the requested tests. Did you see me comments about the |
roschaefer
added a commit
to Human-Connection/Human-Connection
that referenced
this pull request
Oct 22, 2019
@mattwr18 @aonomike You must never `ORDER BY` a property with a `@cypher` directive. Reason: The order by performance will be terribly poor. See my issue: neo4j-graphql/neo4j-graphql-js#239 And my PR: neo4j-graphql/neo4j-graphql-js#247
roschaefer
added a commit
to Human-Connection/Human-Connection
that referenced
this pull request
Oct 22, 2019
@mattwr18 @aonomike You must never `ORDER BY` a property with a `@cypher` directive. Reason: The order by performance will be terribly poor. See my issue: neo4j-graphql/neo4j-graphql-js#239 And my PR: neo4j-graphql/neo4j-graphql-js#247
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #239
First stab at improving order by performance …
At the moment, it's not quite clear to me how to run the tests locally.
I will let the build server run the tests for now and I hope to see some
tests failing that would give me some direction.
@johnymontana @jexp @michaeldgraham I would be very glad if you could
help me out on how to run tests locally and what test cases you would
expect to see.
This pull request is still work-in-progress.