Bug 961011
Summary: | Docs: need to document bnf for selectors | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | Red Hat Enterprise MRG | Reporter: | Joshua Wulf <jwulf> | ||||||
Component: | Messaging_Programming_Reference | Assignee: | Jared MORGAN <jmorgan> | ||||||
Status: | CLOSED CURRENTRELEASE | QA Contact: | Zdenek Kraus <zkraus> | ||||||
Severity: | medium | Docs Contact: | |||||||
Priority: | low | ||||||||
Version: | 1.2 | CC: | 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: |
|
Comment 1
Justin Ross
2013-05-15 14:54:28 UTC
Added to MRG Programming Reference: http://deathstar1.usersys.redhat.com:3000/builds/19948-Messaging_Programming_Reference/#sect-Server-side_Selectors 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 (13) grammar does not allow an empty string to be a selector (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. 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. (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 | "" (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. (13) please see Bug 1082591 comment #3 (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. (13) I'm fine with the note about whitespaces and empty selectors. 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 New Issues: (16) Please add the note in [ESCAPE LiteralString] clause is the LiteralString limitted to a one character string. Updated with changes, please review: http://deathstar1.usersys.redhat.com:3000/builds/19948-Messaging_Programming_Reference/#sect-Server-side_Selectors (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. Can I get this as a patch please? 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 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.
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).
@astitcher: Thanks for the review. @jwulf: code looks fine, but you haven't add the Note 5 from Comment 18 -> ASSIGNED Also make the first comment as additional note, to keep it unified, please. 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' 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 That is correct. -> VERIFIED |