Bug 961011

Summary: Docs: need to document bnf for selectors
Product: Red Hat Enterprise MRG Reporter: Joshua Wulf <jwulf>
Component: Messaging_Programming_ReferenceAssignee: Jared MORGAN <jmorgan>
Status: CLOSED CURRENTRELEASE QA Contact: Zdenek Kraus <zkraus>
Severity: medium Docs Contact:
Priority: low    
Version: 1.2CC: astitcher, esammons, gsim, jross, mmurray, tross, zkraus
Target Milestone: 3.0   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: 510129 Environment:
Last Closed: 2015-01-22 15:28:11 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 510129    
Bug Blocks: 957981    
Attachments:
Description Flags
Original text, changed text and diff
none
This is the corrected syntax accepted by the selector parser none

Comment 1 Justin Ross 2013-05-15 14:54:28 UTC
*** Bug 510129 has been marked as a duplicate of this bug. ***

Comment 3 Zdenek Kraus 2014-03-31 12:10:47 UTC
we've found several issues with this grammar:

(1) from the Constraint for the keywords the 'ESCAPE' keyword missing

(2) Could you please drop the in-grammar comments and make them as notes below ?

(3) LiteralString comment about embedded quote is insufficiently describe, please add detailed note, that the repeating (+) of that pattern enables usage of single quote in literalstring in form of " '' " which then tranforms into the one single quote.

(4) please change the note about the case insensitivity to general note, also the 'E' in the exponent and also the 'TRUE', 'FALSE' are case insensitive

(5) there are unwanted space in ::= symbol in following rules: 'AndExpression', 'AddExpresion', and 'MultiplyExpression'

(6) in MultiplyOps, there are mityped "-", it should be "/"

(7) there are optionality operator _?_ missing at the end of following rules:
'OrExpression', 'AndExpression', 'AddExpression', and 'MultiplyExpression'

(8) change the brackets [ ] to ( )? to preserve consistency in following rules: 'Exponent', and 'LinearApproxNumeric', OR change the other occurences of ( ) and ( )? which are ment to be optional to brackets [ ]

(9) the rule for "IN" keyword missing

(10) please remove the "NOT" "BETWEEN" and "NOT" "LIKE" rule, because they are redundant, same effect could be achieved by applying the _"NOT" ComparisonExpression_ rule

(11) please remove the "IS" "NOT" NULL rule and incorporate it into the "IS" "NULL" rule as "IS" ("NOT")? "NULL"

(12) please remove the unnecesary braces ( ) at rule 'LiteralApproxNumeric'


-> ASSIGNED

Comment 4 Zdenek Kraus 2014-03-31 12:14:28 UTC
(13) grammar does not allow an empty string to be a selector

Comment 5 Joshua Wulf 2014-04-01 04:39:50 UTC
(1) Added ESCAPE
(2) Changed the formatting of the comments and also made them notes below. How does that look? I can take the comments out of the grammar altogether if the formatting doesn't do it.
(3) Added explanatory note as Note 2.
(4) Added as Note 1
(5) Removed spaces.
(6) Changed - to /
(7) Added ? to end of rules. Not sure if it should be _?_ or ?.
(8) Changed [ ] to ( )
(9) Need a rule supplied.
(10) Need engineering ack .
(11) Need rule supplied and engineering ack.
(12) Removed.
(13) Unclear on action required.

Comment 6 Joshua Wulf 2014-04-01 04:42:56 UTC
Andrew, can you help with items 9, 10, 11 (and possibly 13)?

The canonical source of the grammar atm is http://svn.apache.org/viewvc/qpid/tags/0.22/qpid/cpp/src/qpid/broker/SelectorExpression.cpp?revision=1492230&view=markup

I can make changes to the grammar in the docs, but I need a canonical source to cite. A comment in this bug is fine, or if the upstream grammar gets updated I can rebase on the new revision.

Comment 7 Zdenek Kraus 2014-04-01 06:11:47 UTC
(1) OK
(2) This looks good, maybe if the // Note # could be (try it and decide) few spaces away of the grammar
(3) OK
(4) OK
(5) OK
(6) OK
(7) the ? is correct, sorry for adding my emphasizing
but please fix it from rules 'AddOps', and 'MultiplyOps' to 'AddExpression', and 'MultiplyExpression', the ? sybol simply says that the Non-terminal before is optional, and if before is the ( ) than all in such braces is optional.


(8) Please add the ? to the ( ) at rules 'Exponent', 'LiteralApproxNumeric'
I also overlooked two optional parts in ComparisonExpression with the "ESCAPE" keyword,
please change the [] also:
AddExpression "NOT"? "LIKE" LiteralString ( "ESCAPE" LiteralString )?

don't add ? to the Constraint clause


(9)for the 'ComparsionExpression' ::= 
add rule:
AndExpression "NOT"? "IN" "(" PrimaryExpression ("," PrimaryExpression)* ")"

"NOT"? or second not-form rule according to (11)


(10) ok, at my second run, this wasn't a valid point, it cannot be achieved by the "NOT" ComparsionExpression, but following the matter of redundant rules to point (11)
CANCELLED

(11)
example of the rules with the optional "NOT"? Terminal to get rid of the redundant rules

ComparsionExpression ::= AddExpression "IS" "NOT"? "NULL" | AddExpression "NOT"? "LIKE" LiteralString ( "ESCAPE LiteralString)? | AddExpression "NOT"? "BETWEEN" AddExpression "AND" AddExpression


(12) OK
(13) Valid selector is also the "" -- empty string, but according to the grammar this is not possible I would add a rule to Start symbol, which is not present yet (14)

Start ::= BoolExpression | ""

New issues:
(14) grammar missing a start symbol, please place it on top as first rule
Start ::= BoolExpression | ""

Comment 8 Andrew Stitcher 2014-04-03 17:52:46 UTC
(In reply to Joshua Wulf from comment #5)
> (8) Changed [ ] to ( )

You needed to change "[" ... "]" to "(" ... ")?" otherwise the meaning changes.

Although it is valid to use "?" by itself to signify the previous element is optional I prefer to always put the braces in for extra clarity.

> (9) Need a rule supplied.
...
    AddExpression  ( "NOT" )? "IN" "(" AddExpression ("," AddExpression)* ")" |
...

[Note that the supplied expression in comment #7 is incorrect.]

> (10) Need engineering ack .
NACK - The original assertion is untrue.

However it does make sense to combine the rules as:
...
    AddExpression ( "NOT" )? "LIKE" LiteralString ( "ESCAPE" LiteralString )? |
    AddExpression ( "NOT" )? "BETWEEN" AddExpression "AND" AddExpression

> (11) Need rule supplied and engineering ack.
...
    AddExpression "IS"  ( "NOT" )? "NULL" |
...

Is a good replacement for the 2 lines.

> (13) Unclear on action required.
See below - this request is disputable.


(In reply to Zdenek Kraus from comment #7)
> (13) Valid selector is also the "" -- empty string, but according to the
> grammar this is not possible I would add a rule to Start symbol, which is
> not present yet (14)

Why do you assert that the empty selector is valid? The evidence seems against you it is neither accepted by the selector parser nor documented in this grammar!

Do you have some other source of information?

> 
> Start ::= BoolExpression | ""

The "start" symbol *is* BoolExpression as implemented in the code - this definitely needs clarifying - I think I would call it "SelectExpression" rather than "BoolExpression" in the grammar and move it to the top or bottom.

Comment 9 Zdenek Kraus 2014-04-04 05:47:11 UTC
(13) please see Bug 1082591 comment #3

Comment 10 Andrew Stitcher 2014-04-04 20:40:47 UTC
(13) I don't really think anything extra is really required in the grammar for this case. It would be better stated in text: Something like -:

"...Whitespace is ignored, ... an empty selector will be interpreted as always true."

Sorry if that is already present somewhere. I think adding it into the grammar explicitly just adds irrelevant noise.

Comment 11 Zdenek Kraus 2014-04-07 04:33:33 UTC
(13) I'm fine with the note about whitespaces and empty selectors.

Comment 13 Zdenek Kraus 2014-04-23 08:48:23 UTC
New issues:
(15) grammar does not support negative and explicit positive numbers, please change the rule:

Literal ::= LiteralBool | LiteralString | LiteralApproxNumeric | LiteralExactNumeric

to:

Literal ::= LiteralBool | LiteralString | (+|-)?LiteralApproxNumeric | (+|-)?LiteralExactNumeric

Comment 14 Zdenek Kraus 2014-04-24 06:09:06 UTC
New Issues:
(16) Please add the note in [ESCAPE LiteralString] clause is the LiteralString limitted to a one character string.

Comment 16 Zdenek Kraus 2014-07-14 07:24:39 UTC
(1) OK
(2) OK
(3) OK
(4) OK
(5) OK
(6) OK
(7) OK
(8) STILL EXISTS
AddExpression "NOT"? "LIKE" LiteralString ( "ESCAPE" LiteralString )?

(9) STILL EXISTS
for the 'ComparsionExpression' ::= 
add rule:
AndExpression "NOT"? "IN" "(" PrimaryExpression ("," PrimaryExpression)* ")"

"NOT"? or second not-form rule according to (11)


(10) CANCELLED

(11) STILL EXISTS
example of the rules with the optional "NOT"? Terminal to get rid of the redundant rules

ComparsionExpression ::= AddExpression "IS" "NOT"? "NULL" | AddExpression "NOT"? "LIKE" LiteralString ( "ESCAPE LiteralString)? | AddExpression "NOT"? "BETWEEN" AddExpression "AND" AddExpression


(12) OK
(13) OK


(14)

according to Comment 8 last note
please change the "BoolExpression" to "SelectExpression" which makes a start symbol of the grammar, so please move it to top of grammar, or as first rule after defining basic symbols -- above Literal rule

(15) Sorry this is my fault, it should be:

Literal ::= LiteralBool | LiteralString | ("+"|"-")?LiteralApproxNumeric | ("+"|"-")?LiteralExactNumeric


(16) Partially, please resolve (8) and incorporate change

Please add the note in (ESCAPE LiteralString)? clause is the LiteralString limitted to a one character string.

New issues:
(17) bad quoting, in rule: Exponent, please change quoting from ' -> "
Exponent ::= ('+'|'-')? LiteralExactNumeric
<to>
Exponent ::= ("+"|"-")? LiteralExactNumeric

(18) bad optional modifier, in rules AddOps, MultiplyOps please remove the ?
AddOps ::= "+" | "-"?
MultiplyOps ::= "*" | "/"?

<to>

AddOps ::= "+" | "-"
MultiplyOps ::= "*" | "/"

(19) missing optional modifiers, in rules AddExpression, MultiplyExpression add the ?
AddExpression ::= MultiplyExpression (  AddOps MultiplyExpression )
MultiplyExpression ::= UnaryArithExpression ( MultiplyOps UnaryArithExpression )

<to>

AddExpression ::= MultiplyExpression (  AddOps MultiplyExpression )?
MultiplyExpression ::= UnaryArithExpression ( MultiplyOps UnaryArithExpression )?

(20) 
please remove the unnecessary empty lines, some of them are welcomed. Please group simmilar rules and definitions together.

Comment 17 Joshua Wulf 2014-08-14 03:27:13 UTC
Can I get this as a patch please?

Comment 18 Zdenek Kraus 2014-08-14 07:06:19 UTC
Created attachment 926682 [details]
Original text, changed text and diff

I cannot find the repo for documentation, so I made a diff on the plain text of the BNF grammar.

please add a Note:

Note 5: AddExpression "NOT"? "LIKE" LiteralString ( "ESCAPE" LiteralString )? clause, there is a constraint of single character for the ESCAPE LiteralString

and please renumber them by the order of appearance.

Comment 19 Andrew Stitcher 2014-08-14 17:54:35 UTC
Comment on attachment 926682 [details]
Original text, changed text and diff

There are errors in the final syntax attached here by @zkraus. I will upload my version of the final syntax.

Comment 20 Andrew Stitcher 2014-08-14 17:59:17 UTC
Created attachment 926873 [details]
This is the corrected syntax accepted by the selector parser

There were a number of small errors and omissions in the previous syntax supplied by @zkraus. Here is a version I have verified against the actual code. I have also fixed the embedded code documentation to be what I attach here (at least on the trunk and probably 0.30 branches of upstream qpid).

Comment 22 Zdenek Kraus 2014-08-15 07:41:56 UTC
@astitcher: Thanks for the review.

@jwulf: code looks fine, but you haven't add the Note 5 from Comment 18

-> ASSIGNED

Comment 23 Zdenek Kraus 2014-08-15 07:43:39 UTC
Also make the first comment as additional note, to keep it unified, please.

Comment 24 Andrew Stitcher 2014-08-15 14:24:09 UTC
The Note numbering isn't quite correct:

There is a reference to Note 5, but no note 5 - I think it should be referring to Note 3. 

I think the current reference to Note 3 should just be deleted.

Note 4 is the note for the first comment.

Note 3 Should also say that the characters '%' and '_' are not allowed.

Note 1 I think 'reserved words' is more accurate than 'literal terms'

Comment 26 Zdenek Kraus 2014-08-18 09:22:59 UTC
Not correct.

Please make the first line note """// If the overall expression is empty it evaluates to true""" as additional note below, keep it unified.

also there is a wrong "Note 1" in:
Constraint : Identifier NOT IN ("NULL", "TRUE", "FALSE", "NOT", "AND", "OR", "BETWEEN", "LIKE", "IN", "IS", "ESCAPE")  // Note 1
... please change to "Note 2"

and please merge the bottom note:
"""Note also that whitespace is ignored, and an empty selector will be interpreted as always true."""
with the firstline note. It makes sence to have it just like one note in the start rule.

->ASSIGNED

Comment 28 Zdenek Kraus 2014-08-19 13:37:58 UTC
That is correct.

-> VERIFIED