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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
