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

3.0 Support REMOVE property in cost planner #6071

Merged
merged 4 commits into from Dec 14, 2015

Conversation

craigtaverner
Copy link
Contributor

Since SET n.prop = null is semantically equivalent to removing a property, we planned this as a rewrite to SetProperty in the ClauseConversion phase.

@pontusmelke
Copy link
Contributor

IMHO it would be nicer to have a proper RemoveProperty logical plan. Looking at the plans makes it feel somewhat inconsistent that we have SetLabels/RemoveLabels but only SetProperty.

val result = updateWithBothPlanners("MATCH (n) REMOVE n.prop RETURN exists(n.prop) as still_there")

//THEN
assertStats(result, propertiesSet = 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably rename this propertiesSet parameter to propertiesWritten or something, because this is a bit confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curiously the stats did not have a propertiesRemoved counter, so as far as they were concerned removal was seen as a set-property. I took this has a hint! :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I saw that too, and that's why I made this comment.

@Mats-SX
Copy link
Contributor

Mats-SX commented Dec 11, 2015

I agree with @pontusmelke, but implementation-wise I see that we can definitely reuse the runtime code (which we can not for labels, as they don't have this symmetry), and implement it as setting null, as long as the plan description in this case prints remove.

@craigtaverner
Copy link
Contributor Author

Regarding the discussion of having a real RemoveProperty or fake RemovePropery in the plan description, should we differentiate between the user typing REMOVE n.prop or SET n.prop = null or SET n.prop = {param_that_is_null}. Currently all cases would look like a SET property. If we do Pontus suggestion, only the first would look like REMOVE, the others like SET. Mats suggestion would make the second one look like REMOVE if there was no auto-parameterization.

@Mats-SX
Copy link
Contributor

Mats-SX commented Dec 11, 2015

I think it's perfectly reasonable to plan Remove for a query where we do see SET n.prop = null. That's even educational for the user.

@craigtaverner
Copy link
Contributor Author

The problem is that this plan makes autoparameterization hide the remove. That seems unreasonable to me. Pontus approach of making REMOVE syntax have REMOVE plan, and SET syntax have SET plan is less confusing. But it is more code and hides the semantic equivalence.

@systay
Copy link
Contributor

systay commented Dec 14, 2015

I think we should strive to have few and simple operators in our query plans. That makes it easier to build planners and runtimes working with these operators. So, in this case, I prefer a single operator that solves both problems. The ability of the user to recognise query parts in the logical plan is much less valuable, IMHO.

craigtaverner and others added 4 commits December 14, 2015 11:30
Since SET n.prop = null is semantically equivalent to removing a property, we planned this as a rewrite to SetProperty in the ClauseConversion phase.
@systay systay merged commit 37014b6 into neo4j:3.0 Dec 14, 2015
@craigtaverner craigtaverner deleted the 3.0-remove-property branch May 13, 2016 11:07
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

5 participants