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.

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

Fix eagerness for labels() function #8575

Merged
merged 5 commits into from Jan 10, 2017
Merged

Conversation

henriknyman
Copy link
Contributor

We need to be eager when the use of labels() function in a query horizon
conflicts with a label write, which requires new conflict checks.

  • For the rule planner a new read effect is introduced
  • For read-write conflicts in the cost planner, a new location to plan eagerness
    is introduced, with a dedicated check and pass, after planning the event horizon.
  • For write-read conflicts in the cost planner, the check is added to the
    existing passes.

Also fixes related eagerness bugs for property reads and extends testing.

Should also work for horizons of query graph tails
Fixes an eagerness bug in both rule and cost planner for relationship
property reads in horizon
We need to be eager when the use of labels() function in a query horizon
conflicts with a label write, which requires new conflict checks.

- For the rule planner a new read effect is introduced
- For read-write conflicts in the cost planner, a new location to plan eagerness
is introduced, with a dedicated check and pass, after planning the event horizon.
- For write-read conflicts in the cost planner, the check is added to the
existing passes.
Copy link
Contributor

@Mats-SX Mats-SX left a comment

Choose a reason for hiding this comment

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

Looks good!

if (tail.queryGraph.writeOnly) false
else
// NOTE: Here we do not check writeOnlyHeadOverlapsHorizon, because we do not know of any case where a
// write-only head could cause problems with reads in future horizons
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a useful observation. I don't know any way for a write-only head to increase cardinality above one, which makes eagerness analysis trivial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that explanation is more on point

} || {
(labelsToSet.nonEmpty || removeLabelPatterns.nonEmpty) &&
horizon.dependingExpressions.exists {
case f: FunctionInvocation if f.function.get == Labels => true
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering whether get here could ever throw exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope not :)

@systay systay merged commit 03880ce into neo4j:3.0 Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants