Skip to content

Fix reorder expression - #3078

Open
nafraf wants to merge 6 commits into
masterfrom
fix-reorder-exp
Open

Fix reorder expression#3078
nafraf wants to merge 6 commits into
masterfrom
fix-reorder-exp

Conversation

@nafraf

@nafraf nafraf commented May 3, 2023

Copy link
Copy Markdown
Contributor

Hi, this patch is to fix #3058.
The idea is to don't score expressions with GhostEdge, because the reorder expression function was removing conditional variable length traverse operations.

Before applying the patch, the reordering of the expression removed the Conditional Variable Length Traverse operation and the module crashes:

127.0.0.1:6379> GRAPH.explain g "MATCH ()-[*0]->(:A)--() RETURN 0"
1) "Results"
2) "    Project"
3) "        Conditional Traverse | (@anon_1:A)->(@anon_2)"
4) "            Node By Label Scan | (@anon_0)"

But for the same pattern written backwards, there was not problem:

127.0.0.1:6379> GRAPH.explain g "MATCH ()--(:A)<-[*0]-() RETURN 0"
1) "Results"
2) "    Project"
3) "        Conditional Variable Length Traverse | (@anon_1)-[@anon_4*0..0]->(@anon_2)"
4) "            Conditional Traverse | (@anon_1)->(@anon_0)"
5) "                Node By Label Scan | (@anon_1:A)"

@nafraf
nafraf marked this pull request as ready for review May 3, 2023 20:15
@nafraf nafraf mentioned this pull request May 4, 2023
Open
@filipecosta90

filipecosta90 commented May 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Automated performance analysis summary

This comment was automatically generated given there is performance data available.

In summary:

  • Detected a total of 19 stable tests between versions.
  • Detected a total of 1 highly unstable benchmarks.

You can check a comparison in detail via the grafana link

Comparison between master and fix-reorder-exp.

Time Period from 30 days ago. (environment used: oss-standalone)

Test Case Baseline master (median obs. +- std.dev) Comparison fix-reorder-exp (median obs. +- std.dev) % change (higher-better) Note
ENTITY_COUNT 33324 +- 0.0% (7 datapoints) 33324 +- nan% (1 datapoints) -0.0% -- no change --
EXPLICIT-EDGE-DELETION 100 +- 0.0% (7 datapoints) 100 +- nan% (1 datapoints) 0.0% -- no change --
GRAPH500-SCALE_18-EF_16-1_HOP 33325 +- 24.4% UNSTABLE (7 datapoints) 33325 +- nan% (1 datapoints) 0.0% UNSTABLE (very high variance)
GRAPH500-SCALE_18-EF_16-1_HOP_MIXED_READ_65perc_WRITE_25perc_DEL_10perc 50 +- 0.0% (7 datapoints) 50 +- nan% (1 datapoints) 0.0% -- no change --
GRAPH500-SCALE_18-EF_16-1_HOP_MIXED_READ_75perc_WRITE_25perc 56 +- 2.2% (7 datapoints) 56 +- nan% (1 datapoints) -0.0% -- no change --
GRAPH500-SCALE_18-EF_16-2_HOP 9999 +- 4.4% (7 datapoints) 9999 +- nan% (1 datapoints) 0.0% -- no change --
GRAPH500-SCALE_18-EF_16-2_HOP_MIXED_READ_75perc_WRITE_25perc 56 +- 2.9% (7 datapoints) 56 +- nan% (1 datapoints) -0.0% -- no change --
GRAPH500-SCALE_18-EF_16-3_HOP 901 +- 1.9% (7 datapoints) 901 +- nan% (1 datapoints) 0.0% -- no change --
GRAPH500-SCALE_18-EF_16-3_HOP_MIXED_READ_75perc_WRITE_25perc 56 +- 2.0% (7 datapoints) 56 +- nan% (1 datapoints) -0.0% -- no change --
IMPLICIT-EDGE-DELETION 17 +- 0.0% (7 datapoints) 17 +- nan% (1 datapoints) -0.0% -- no change --
INSPECT-EDGES 2500 +- 0.0% (7 datapoints) 2500 +- nan% (1 datapoints) -0.0% -- no change --
MIX_READ_WRITE 10000 +- 0.0% (7 datapoints) 10000 +- nan% (1 datapoints) -0.0% -- no change --
NODE-BATCH-DELETE 50 +- 0.0% (7 datapoints) 50 +- nan% (1 datapoints) 0.0% -- no change --
NODE-INDEX-LOOKUP 33 +- 0.0% (7 datapoints) 33 +- nan% (1 datapoints) 0.0% -- no change --
SORT_ENTITIES 100 +- 0.0% (7 datapoints) 100 +- nan% (1 datapoints) -0.0% -- no change --
UPDATE-BASELINE 24999 +- 0.0% (7 datapoints) 25000 +- nan% (1 datapoints) 0.0% -- no change --
VARIABLE-LENGTH-FILTER 33325 +- 0.0% (7 datapoints) 33325 +- nan% (1 datapoints) -0.0% -- no change --
VARIABLE_LENGTH_EXPAND_INTO 100 +- 0.0% (7 datapoints) 100 +- nan% (1 datapoints) 0.0% -- no change --
allShortestPaths-10hop-100Kpaths 30 +- 4.4% (7 datapoints) 30 +- nan% (1 datapoints) 0.0% -- no change --
allShortestPaths-4hop-10Kpaths 333 +- 3.1% (7 datapoints) 333 +- nan% (1 datapoints) 0.0% -- no change --

@codecov

codecov Bot commented Jul 20, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (86d6c12) 90.85% compared to head (1bffce1) 90.88%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3078      +/-   ##
==========================================
+ Coverage   90.85%   90.88%   +0.02%     
==========================================
  Files         294      294              
  Lines       30269    30269              
==========================================
+ Hits        27502    27509       +7     
+ Misses       2767     2760       -7
Impacted Files Coverage Δ
...xecution_plan/optimizations/traverse_order_utils.c 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gkorland gkorland mentioned this pull request Sep 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Crash due to untrue assertion AlgebraicExpression_DiagonalOperand

2 participants

@nafraf @filipecosta90