Re: [PATCH] Re: Adding XMLEXISTS to the grammar

Hi Peter,

Thanks for your feedback.

On 20/07/10 19:54, Peter Eisentraut wrote:
>>> Attached is a patch with the revised XMLEXISTS function, complete with
>>> grammar support and regression tests. The implemented grammar is:
>>>
>>> XMLEXISTS ( xpath_expression PASSING BY REF xml_value [BY REF] )
>>>
>>> Though the full grammar makes everything after the xpath_expression
>>> optional, I've left it has mandatory simply to avoid lots of rework of
>>> the function (would need new null checks, memory handling would need
>>> reworking).
> Some thoughts, mostly nitpicks:
>
> The snippet of documentation could be clearer.  It says "if the xml
> satisifies the xpath".  Not sure what that means exactly.  An XPath
> expression, by definition, returns a value.  How is that value used to
> determine the result?
>    

I'll rephrase it: The function xmlexists returns true if the xpath 
returns any nodes and false otherwise.

> Naming of parser symbols: xmlexists_list isn't actually a list of
> xmlexists's.  That particular rule can probably be done away with anyway
> and the code be put directly into the XMLEXISTS rule.
>
> Why is the first argument AexprConst instead of a_expr?  The SQL
> standard says it's a character string literal, but I think we can very
> well allow arbitrary expressions.
>    

Yes, it was AexprConst because of the specification. I also found that 
using it solved my shift/reduce problems, but I can change it a_expr as 
see if I can work them out in a different way.

> xmlexists_query_argument_list should be optional.
>    

OK, I'll change it.

> The rules xml_default_passing_mechanism and xml_passing_mechanism are
> pretty useless to have a separate rules.  Just mention the tokens where
> they are used.
>    

Again, I'll change that too.

> Why c_expr?
>    

As with the AexprConst, it's choice was partially influenced by the fact 
it solved the shift/reduce errors I was getting. I'm guessing than that 
I should really use a_expr and resolve the shift/reduce problem differently?

> Call the C-level function xmlexists for consistency.
>    

Sure. I'll look to get a patch addressing these concerns out in the next 
day or two, work/family/sleep permitting! :)

Regards,

--
Mike Fowler
Registered Linux user: 379787