CIP and Scenarios for precedence rules #502

hvub · 2021-08-11T14:14:51Z

Based on PR #522

This PR aims to add

A CIP stating the precedence rules of openCypher -- already, (rendered view)
Grammar refactorings
- Add <PatternPredicate> ::= <RelationshipsPattern> for clearer distinction of <RelationshipsPattern> appearing as an <Atom>
- Add separate production of s for better referencing and separation of quantifiers in the grammar
Adjustment of CIP2015-05-13-EXISTS to grammar refactorings, (rendered view)
TCK scenarios that test the precedence rules of openCypher

cip/1.accepted/CIP2021-08-10-Operator-precedence.adoc

martin-neotech

Thanks. Got some comments :)

cip/1.accepted/CIP2021-08-10-Operator-precedence.adoc

martin-neotech

Thanks

hedengran

Great work on this! We have a few minor suggestions and questions, and then we found (what we believe) is one bigger problem. Let us know what you think.

Co-reviwed-by: Nadja Müller @nadja-muller

hedengran · 2021-12-01T09:47:24Z

cip/1.accepted/CIP2021-08-10-Operator-precedence.adoc

+* https://raw.githack.com/openCypher/openCypher/master/tools/grammar-production-links/grammarLink.html?p=ListComprehension[List comprehensions]
+* https://raw.githack.com/openCypher/openCypher/master/tools/grammar-production-links/grammarLink.html?p=PatternComprehension[Pattern comprehensions]
+* https://raw.githack.com/openCypher/openCypher/master/tools/grammar-production-links/grammarLink.html?p=Reduce[Reduce operator]
+* quantifiers


Do you maybe want to expand on the definition of these? I'm assuming it's referring to All(), Any(), Single() and None()?

Quantifiers do not have their own grammar production has they are not really referencable like the other things. I can

a. Just extend the text here a bit, or
b. Modify the grammar so that they have their own production

The latter is the bigger change, but I still lean towards it, because it is a bit of an oddity that the grammar does not separate the quantifiers out into their own production. Opinions?

I don't have a strong opinion, as long as we clarify what those quantifiers are. For what it's worth, Neo4j does the latter.

Done. Note that link does not work yet, since it always goes the master branch.

cip/1.accepted/CIP2021-08-10-Operator-precedence.adoc

hedengran · 2021-12-01T10:01:51Z

cip/1.accepted/CIP2021-08-10-Operator-precedence.adoc

+|Level         |Group         |Operators
+
+|12
+|Atoms


Is ShortestPathPattern missing from this table?

Is MapProjection not part of openCypher?

Regarding map projections, that is a good question. Somethings were excluded back then. There is no trace of map projections in the grammar and the TCK, as far as I tell based on a quick look. However, the Cypher 9 Reference has them. So it is a mixed picture.

Shortest paths have a similarly mixed picture. The reference and the grammar have them as part of pattern in the match clause, the TCK does not have scenario on them. Shortest path in expressions, which is the bit relevant here, is non of the oC artifacts.

It is probably worth asking CLG about these two things.

hedengran · 2021-12-01T10:05:55Z

cip/1.accepted/CIP2021-08-10-Operator-precedence.adoc

+* https://raw.githack.com/openCypher/openCypher/master/tools/grammar-production-links/grammarLink.html?p=PatternComprehension[Pattern comprehensions]
+* https://raw.githack.com/openCypher/openCypher/master/tools/grammar-production-links/grammarLink.html?p=Reduce[Reduce operator]
+* quantifiers
+* https://raw.githack.com/openCypher/openCypher/master/tools/grammar-production-links/grammarLink.html?p=RelationshipsPattern[Pattern predicates]


Is pattern predicates the correct term?

For me a naked pattern appearing as an expression is a pattern predicate. The oC grammar does not have an extra production for pattern predicates, because these have the same syntax as the RelationshipsPattern non-terminal. However, the RelationshipsPattern non-terminal is not only used for pattern predicates but also in pattern comprehensions. Hence, we can not rename the RelationshipsPattern non-terminal to pattern predicate.

In other words, when appearing as an atom a RelationshipsPattern is a (or plays the role of a) pattern predicate. Again, adding an extra production to the grammar would clarify this picture a bit.

I see. I was more confused on the term "pattern predicates" because of GPMs "node pattern predicates" and "relationship pattern predicates".

Is a naked pattern appearing as an expression generally referred to as "pattern predicate" in oC? It's easy to mix up with the GPM predicates.

Well, generally. Terminology evolves with understanding and with people. It is in far a more recent term that we, i.e. the CLG, took the decision that naked patterns in expressions should evaluate to a boolean (rather than a list of paths, as they used to). It was argued, that patterns in match are also understood as predicates. So pattern elsewhere in the language should also behave like predicates. In general, it something evaluates to boolean it is reasonable to call it a predicates.

Anyway, it may be good to encode this name in the oC grammar. I am happy to do that. It is a conclusion that is somewhat similar to the one we have to quantifiers. :-)

Done. I have also updated CIP2015-05-13-EXISTS to grammar refactorings (rendered view), which provides a clear introduction point of the term "pattern predicate" now. It used the term already before, but now it does so much more prominently.

hedengran · 2021-12-01T10:12:32Z

cip/1.accepted/CIP2021-08-10-Operator-precedence.adoc

+|===
+|Level         |Group         |Operators
+
+|12


I think the levels are usually the other way around, where 1 has highest precedence (see e.g. C++ Operator Precedence).

Depends who you ask (see, e.g. https://introcs.cs.princeton.edu/java/11precedence/ or https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence#table). It is likewise confusing to say the highest has the lowest number as it is confusing to have the highest level shown lowest in the table.

Probably for exactly this reason, many lists do not given any numbering at all. We can do the same here.

Fair enough! I don't have a strong opinion, but I did find it convenient to have a level number to refer to when doing the reviewing. So I'd suggest keep it as is.

As a minimal improvement, I have add a note before the table saying

The higher the level number the higher the precedence.

Hopefully that helps to avoid confusion.

hedengran · 2021-12-01T10:53:46Z

cip/1.accepted/CIP2021-08-10-Operator-precedence.adoc

+.3+|10
+|https://raw.githack.com/openCypher/openCypher/master/tools/grammar-production-links/grammarLink.html?p=StringOperatorExpression[String operators] (left-hand operand)
+|
+
+* Prefix predicate (and right-hand operand)
+* Suffix predicate (and right-hand operand)
+* Contains predicate (and right-hand operand)
+* Regular expression predicate
+
+|https://raw.githack.com/openCypher/openCypher/master/tools/grammar-production-links/grammarLink.html?p=ListOperatorExpression[List operators] (left-hand operand)
+|
+
+* List element containment predicate (and right-hand operand)
+* List element access
+* List slicing
+
+|https://raw.githack.com/openCypher/openCypher/master/tools/grammar-production-links/grammarLink.html?p=NullOperatorExpression[Null predicates] (left-hand operand)
+|
+
+* Null predicate
+* Not-null predicate


This level is very different from the Neo4j implementation. In Neo4j we basically have that:

List operators List element access and List slicing are part of level 11

List operator List element containment predicate go between 5 and 6

Null predicates and string predicates go between 5 and 6

So level 10 does not exist, and the elements in level 10 (excluding the list element access and slicing) constructs a new level between 5 and 6 in Neo4j.

The order (simplified) would be:

atoms

graph element operators, list element access and slicing

arithmetic operators

predicates (that's why we think list predicates and null predicates should go between 5 and 6)

boolean expressions

Looking at "what others do" (which you covered further down), it seems like e.g. Python has a similar distinction as Neo4j.

A couple of examples

Null predicate and arithmetic additive operators

RETURN null + null IS NULL

Above example evaluates to true in Neo4j. With the precedence stated in this document, it would evaluate to null (null + (null IS NULL) -> null + true -> null).

List element containment predicate and arithmetic additive inverse

RETURN -1 IN [1]

Above evaluates to false in Neo4j. With the precedence stated in this document, it would fail (since it would be something like -(1 IN [1]) -> -true, which is nonsensical).

It makes sense what you describe. As far as I can tell, openCypher has simply been under defined with regards to precedence. Since this CIP refers to the grammar wrt. the levels, it seems appropriate to adjust the grammar accordingly.

hedengran · 2022-05-09T10:12:50Z

tck/features/expressions/precedence/Precedence1.feature

+           collect((a <boolop> b <pred> c) <> ((a <boolop> b) <pred> c)) AS neq
+      RETURN none(x IN eq WHERE NOT x) AND any(x IN neq WHERE x) AS result


I tried running this test in Neo4j and it fails on the following examples:

| pred | boolop | | = | XOR | | >= | OR | | > | AND | | <> | XOR |

I have checked all four combinations. These operator pairings are indeed associative on truth values. I have commented them out and added scenarios that test the associativity in turn, i.e. test that indeed no precedence rules apply.

tck/features/expressions/precedence/Precedence3.feature

hedengran · 2022-05-09T10:27:35Z

tck/features/expressions/precedence/Precedence1.feature

+      UNWIND [true, false, null] AS c
+      WITH collect((a OR b XOR c) = (a OR (b XOR c))) AS eq,
+           collect((a OR b XOR c) <> ((a OR b) XOR c)) AS neq
+      RETURN none(x IN eq WHERE NOT x) AND any(x IN neq WHERE x) AS result


It's a matter of taste, but I find the double negation a bit confusing. This reads better to me:

Suggested change

RETURN none(x IN eq WHERE NOT x) AND any(x IN neq WHERE x) AS result

RETURN all(x IN eq WHERE x) AND any(x IN neq WHERE x) AS result

Co-authored-by: Gustav Hedengran <[email protected]>

hedengran

Massive work!

hedengran · 2022-06-03T11:56:08Z

tck/features/expressions/precedence/Precedence1.feature

@@ -476,4 +517,4 @@ Feature: Precedence1 - On boolean values
      | boolop |
      | OR     |
      | XOR    |
-      | AND    |
+      | AND    |


Nitpicking, but there should actually be a newline terminating the file, right?

Yes! Good catch. Thanks.

martin-neotech reviewed Aug 31, 2021

View reviewed changes

cip/1.accepted/CIP2021-08-10-Operator-precedence.adoc Outdated Show resolved Hide resolved

hvub referenced this pull request in hvub/openCypher Sep 16, 2021

Add null case to Boolean4.[2]

cf3efe8

hvub force-pushed the scenarios-for-precedence-rules branch from 5417650 to 8363a11 Compare September 16, 2021 21:56

martin-neotech suggested changes Oct 1, 2021

View reviewed changes

martin-neotech approved these changes Nov 1, 2021

View reviewed changes

arnefischereit assigned hedengran Nov 23, 2021

hedengran suggested changes Dec 1, 2021

View reviewed changes

hvub force-pushed the scenarios-for-precedence-rules branch from ac7391c to b8ff397 Compare December 3, 2021 16:37

hvub force-pushed the scenarios-for-precedence-rules branch from a37c25b to 0f75ca7 Compare January 13, 2022 09:29

hvub force-pushed the scenarios-for-precedence-rules branch from 0f75ca7 to 5ddb194 Compare March 25, 2022 15:31

hvub added 20 commits April 8, 2022 11:12

CIP intro

e734a61

some intro refinement

253c862

precedence table based on grammar

e384fc7

precedence table based on grammar

2ac7248

precedence level section

5b5ecf0

some copyediting

3533ae9

some more copyediting

beaa8a1

some more copyediting

b68cbba

precedence in chains

ab277c0

Examples for associative operators

5e1ccec

some copyediting

735fa78

more examples and some refactoring

8e70ea8

final test bits and some clean up

bd424ac

fix typo

6c0a9be

Fix typo

b8a6252

Editorial fixes

986d7b0

Editorial fixes.

26e374c

Editorial fixes.

1241126

Editorial fixes.

bbfc5f8

Editorial fixes.

26e836c

hvub added 18 commits April 8, 2022 11:12

Add quantifier production

30be654

some reordering of productions

6f1cf6d

properly refer to <Quantifiers> and <PatternPredicate>

6f77c99

adjust to grammar refactoring

4e011f6

add a note clarifying the level numbers

c0aab32

adjust precedence level table

0f5dd40

rename StringOperatorExpression to StringPredicateExpression

88f3994

rename NullOperatorExpression to NullPredicateExpression

ee67951

Fix precedence of list ops in grammar CIP

b10278f

fix example with signed int

e6ea181

fix example with signed int

9ef40db

fix typo

00b3d65

fix typo

44414b3

scenarios of precedence on boolean values

a6c92d0

scenarios of precedence on numeric values

25c77f0

update license header to 2022

ff8edd8

Fix scenario name duplicate

24d177f

Fix scenario name duplicate

3a72741

hvub force-pushed the scenarios-for-precedence-rules branch from fa5a4f3 to 3a72741 Compare April 8, 2022 09:13

hvub added 3 commits May 2, 2022 17:05

Add precedence scenarios on list values

9f76d1e

Add precedence scenarios on null value

31f3c88

Fixing grammar and style errors

a0e0f63

hedengran suggested changes May 9, 2022

View reviewed changes

hvub and others added 3 commits May 25, 2022 12:10

Accept review suggestion

d68ead0

Co-authored-by: Gustav Hedengran <[email protected]>

Fixing double negation

7d5c454

Fixing failing scenarios

3e65414

hedengran approved these changes Jun 3, 2022

View reviewed changes

add missing newline.

e4b716f

hvub merged commit fdfab2d into opencypher:master Jun 7, 2022

hvub deleted the scenarios-for-precedence-rules branch July 8, 2022 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CIP and Scenarios for precedence rules #502

CIP and Scenarios for precedence rules #502

hvub commented Aug 11, 2021 •

edited

Loading

martin-neotech left a comment

martin-neotech left a comment

hedengran left a comment

hedengran Dec 1, 2021

hvub Dec 1, 2021

hedengran Dec 3, 2021

hvub Dec 3, 2021

hedengran Dec 1, 2021

hedengran Dec 1, 2021

hvub Dec 1, 2021

hedengran Dec 1, 2021

hvub Dec 1, 2021

hedengran Dec 3, 2021

hvub Dec 3, 2021

hvub Dec 3, 2021

hedengran Dec 1, 2021

hvub Dec 1, 2021

hedengran Dec 3, 2021

hvub Dec 3, 2021

hedengran Dec 1, 2021 •

edited

Loading

hvub Dec 1, 2021

hedengran May 9, 2022

hvub May 25, 2022

hedengran May 9, 2022

hvub May 25, 2022

hedengran left a comment

hedengran Jun 3, 2022

hvub Jun 3, 2022

hvub Jun 3, 2022

		collect((a <boolop> b <pred> c) <> ((a <boolop> b) <pred> c)) AS neq
		RETURN none(x IN eq WHERE NOT x) AND any(x IN neq WHERE x) AS result

	RETURN none(x IN eq WHERE NOT x) AND any(x IN neq WHERE x) AS result
	RETURN all(x IN eq WHERE x) AND any(x IN neq WHERE x) AS result

CIP and Scenarios for precedence rules #502

CIP and Scenarios for precedence rules #502

Conversation

hvub commented Aug 11, 2021 • edited Loading

martin-neotech left a comment

Choose a reason for hiding this comment

martin-neotech left a comment

Choose a reason for hiding this comment

hedengran left a 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

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

hedengran Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

A couple of examples

Null predicate and arithmetic additive operators

List element containment predicate and arithmetic additive inverse

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

hedengran left a 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

hvub commented Aug 11, 2021 •

edited

Loading

hedengran Dec 1, 2021 •

edited

Loading