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

2010-08-04 Thread Peter Eisentraut 
On lör, 2010-07-24 at 20:32 +0100, Mike Fowler wrote:
 Attached is the revised version of the patch addressing all the
 issues 
 raised in the review, except for the use of AexprConst and c_expr.
 With 
 my limited knowledge of bison I've failed to resolve the shift/reduce 
 errors that are introduced by using a_expr. I'm open to suggestions
 as 
 my desk is getting annoyed with me beating it in frustration!

It's committed now.  I ended up refactoring this a little bit so that
xpath() and xmlexists() can share more of the code.  Also, I relaxed the
grammar a bit for better compatibility with DB2, Oracle, and Derby.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2010-07-24 Thread Mike Fowler 

On 21/07/10 08:33, Mike Fowler wrote:


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.


[snip]


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?



Attached is the revised version of the patch addressing all the issues 
raised in the review, except for the use of AexprConst and c_expr. With 
my limited knowledge of bison I've failed to resolve the shift/reduce 
errors that are introduced by using a_expr. I'm open to suggestions as 
my desk is getting annoyed with me beating it in frustration!


Thanks again for taking the time to review my work.

Regards,

--
Mike Fowler
Registered Linux user: 379787
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 8554,8562  SELECT xmlagg(x) FROM (SELECT * FROM test ORDER BY y DESC) AS tab;
  ]]/screen
  /para
 /sect3
  
!sect3
  titleXML Predicates/title
  
  indexterm
   primaryIS DOCUMENT/primary
--- 8554,8570 
  ]]/screen
  /para
 /sect3
+/sect2
  
!sect2
  titleXML Predicates/title
+ 
+ indexterm
+  primaryXML Predicates/primary
+ /indexterm
+  
+sect3
+ titleIS DOCUMENT/title
  
  indexterm
   primaryIS DOCUMENT/primary
***
*** 8574,8579  SELECT xmlagg(x) FROM (SELECT * FROM test ORDER BY y DESC) AS tab;
--- 8582,8619 
   between documents and content fragments.
  /para
 /sect3
+
+sect3
+ titleXMLEXISTS/title
+ 
+ indexterm
+  primaryXMLEXISTS/primary
+ /indexterm
+ 
+ synopsis
+ functionXMLEXISTS/function(replaceablexpath/replaceable optionalPASSING BY REF replaceablexml/replaceable optionalBY REF/optional/optional)
+ /synopsis
+ 
+ para
+  The function functionxmlexists/function returns true if the replaceablexpath/replaceable 
+  returns any nodes and false otherwise. If no replaceablexml/replaceable is passed, the function
+  will return false as a XPath cannot be evaluated without content. See the
+  ulink url=http://www.w3.org/TR/xpath/#section-Introduction;W3C recommendation 'XML Path Language'/ulink
+  for a detailed explanation of why.
+ /para
+ 
+ para
+  Example:
+  screen![CDATA[
+ SELECT xmlexists('//town[text() = ''Toronto'']' PASSING BY REF 'townstownToronto/towntownOttawa/town/towns');
+ 
+  xmlexists
+ 
+  t
+ (1 row)
+ ]]/screen
+ /para
+/sect3
/sect2
  
sect2 id=functions-xml-processing
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***
*** 423,431  static TypeName *TableFuncTypeName(List *columns);
  %type list	opt_check_option
  
  %type target	xml_attribute_el
! %type list	xml_attribute_list xml_attributes
  %type node	xml_root_version opt_xml_root_standalone
! %type ival	document_or_content
  %type boolean xml_whitespace_option
  
  %type node 	common_table_expr
--- 423,432 
  %type list	opt_check_option
  
  %type target	xml_attribute_el
! %type list	xml_attribute_list xml_attributes xmlexists_list
  %type node	xml_root_version opt_xml_root_standalone
! %type node	xmlexists_query_argument_list
! %type ival	document_or_content 
  %type boolean xml_whitespace_option
  
  %type node 	common_table_expr
***
*** 511,523  static TypeName *TableFuncTypeName(List *columns);
  	OBJECT_P OF OFF OFFSET OIDS ON ONLY OPERATOR OPTION OPTIONS OR
  	ORDER OUT_P OUTER_P OVER OVERLAPS OVERLAY OWNED OWNER
  
! 	PARSER PARTIAL PARTITION PASSWORD PLACING PLANS POSITION
  	PRECEDING PRECISION PRESERVE PREPARE PREPARED PRIMARY
  	PRIOR PRIVILEGES PROCEDURAL PROCEDURE
  
  	QUOTE
  
! 	RANGE READ REAL REASSIGN RECHECK RECURSIVE REFERENCES REINDEX
  	RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA RESET RESTART
  	RESTRICT RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROW ROWS RULE
  
--- 512,524 
  	OBJECT_P OF OFF OFFSET OIDS ON ONLY OPERATOR OPTION OPTIONS OR
  	ORDER OUT_P OUTER_P OVER OVERLAPS OVERLAY OWNED OWNER
  
! 	PARSER PARTIAL PARTITION PASSING PASSWORD PLACING PLANS POSITION
  	PRECEDING PRECISION PRESERVE PREPARE PREPARED PRIMARY
  	PRIOR PRIVILEGES PROCEDURAL PROCEDURE
  
  	QUOTE
  
! 	RANGE READ REAL REASSIGN RECHECK RECURSIVE REF REFERENCES REINDEX
  	RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA RESET RESTART
  	RESTRICT RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROW ROWS RULE
  
***
*** 539,545  static TypeName *TableFuncTypeName(List *columns);
  
  	WHEN WHERE WHITESPACE_P WINDOW WITH WITHOUT WORK WRAPPER WRITE

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

2010-07-21 Thread Mike Fowler 

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

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

2010-07-20 Thread Peter Eisentraut 
On tis, 2010-06-29 at 12:22 +0100, Mike Fowler wrote:
 Mike Fowler wrote:  
  Thanks again for your help Robert, turns out the fault was in the 
  pg_proc entry (the 3 up there should've been a two!). Once I took the 
  grammar out it was quickly obvious where I'd gone wrong.
 
  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).
 
 
 As with the xpath_exists patch I've now added the SGML documentation 
 detailing this function and extended the regression test a little to 
 test XML literals.

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?

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.

xmlexists_query_argument_list should be optional.

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.

Why c_expr?

Call the C-level function xmlexists for consistency.





-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2010-06-29 Thread Mike Fowler 
Mike Fowler wrote:  
Thanks again for your help Robert, turns out the fault was in the 
pg_proc entry (the 3 up there should've been a two!). Once I took the 
grammar out it was quickly obvious where I'd gone wrong.


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).




As with the xpath_exists patch I've now added the SGML documentation 
detailing this function and extended the regression test a little to 
test XML literals.


Regards,

--
Mike Fowler
Registered Linux user: 379787

*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 8554,8562  SELECT xmlagg(x) FROM (SELECT * FROM test ORDER BY y DESC) AS tab;
  ]]/screen
  /para
 /sect3
  
!sect3
  titleXML Predicates/title
  
  indexterm
   primaryIS DOCUMENT/primary
--- 8554,8570 
  ]]/screen
  /para
 /sect3
+/sect2
  
!sect2
  titleXML Predicates/title
+ 
+ indexterm
+  primaryXML Predicates/primary
+ /indexterm
+  
+sect3
+ titleIS DOCUMENT/title
  
  indexterm
   primaryIS DOCUMENT/primary
***
*** 8574,8579  SELECT xmlagg(x) FROM (SELECT * FROM test ORDER BY y DESC) AS tab;
--- 8582,8616 
   between documents and content fragments.
  /para
 /sect3
+
+sect3
+ titleXMLEXISTS/title
+ 
+ indexterm
+  primaryXMLEXISTS/primary
+ /indexterm
+ 
+ synopsis
+ functionXMLEXISTS/function(replaceablexpath/replaceable PASSING BY REF replaceablexml/replaceable optionalBY REF/optional)
+ /synopsis
+ 
+ para
+  The function functionxmlexists/function returns true if the replaceablexml/replaceable 
+  satisfies the replaceablexpath/replaceable and false otherwise.
+ /para
+ 
+ para
+  Example:
+  screen![CDATA[
+ SELECT xmlexists('//town[text() = ''Toronto'']' PASSING BY REF 'townstownToronto/towntownOttawa/town/towns');
+ 
+  xmlexists
+ 
+  t
+ (1 row)
+ ]]/screen
+ /para
+/sect3
/sect2
  
sect2 id=functions-xml-processing
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***
*** 423,431  static TypeName *TableFuncTypeName(List *columns);
  %type list	opt_check_option
  
  %type target	xml_attribute_el
! %type list	xml_attribute_list xml_attributes
  %type node	xml_root_version opt_xml_root_standalone
! %type ival	document_or_content
  %type boolean xml_whitespace_option
  
  %type node 	common_table_expr
--- 423,432 
  %type list	opt_check_option
  
  %type target	xml_attribute_el
! %type list	xml_attribute_list xml_attributes xmlexists_list
  %type node	xml_root_version opt_xml_root_standalone
! %type node	xmlexists_query_argument_list xml_default_passing_mechanism xml_passing_mechanism
! %type ival	document_or_content 
  %type boolean xml_whitespace_option
  
  %type node 	common_table_expr
***
*** 511,523  static TypeName *TableFuncTypeName(List *columns);
  	OBJECT_P OF OFF OFFSET OIDS ON ONLY OPERATOR OPTION OPTIONS OR
  	ORDER OUT_P OUTER_P OVER OVERLAPS OVERLAY OWNED OWNER
  
! 	PARSER PARTIAL PARTITION PASSWORD PLACING PLANS POSITION
  	PRECEDING PRECISION PRESERVE PREPARE PREPARED PRIMARY
  	PRIOR PRIVILEGES PROCEDURAL PROCEDURE
  
  	QUOTE
  
! 	RANGE READ REAL REASSIGN RECHECK RECURSIVE REFERENCES REINDEX
  	RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA RESET RESTART
  	RESTRICT RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROW ROWS RULE
  
--- 512,524 
  	OBJECT_P OF OFF OFFSET OIDS ON ONLY OPERATOR OPTION OPTIONS OR
  	ORDER OUT_P OUTER_P OVER OVERLAPS OVERLAY OWNED OWNER
  
! 	PARSER PARTIAL PARTITION PASSING PASSWORD PLACING PLANS POSITION
  	PRECEDING PRECISION PRESERVE PREPARE PREPARED PRIMARY
  	PRIOR PRIVILEGES PROCEDURAL PROCEDURE
  
  	QUOTE
  
! 	RANGE READ REAL REASSIGN RECHECK RECURSIVE REF REFERENCES REINDEX
  	RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA RESET RESTART
  	RESTRICT RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROW ROWS RULE
  
***
*** 539,545  static TypeName *TableFuncTypeName(List *columns);
  
  	WHEN WHERE WHITESPACE_P WINDOW WITH WITHOUT WORK WRAPPER WRITE
  
! 	XML_P XMLATTRIBUTES XMLCONCAT XMLELEMENT XMLFOREST XMLPARSE
  	XMLPI XMLROOT XMLSERIALIZE
  
  	YEAR_P YES_P
--- 540,546 
  
  	WHEN WHERE WHITESPACE_P WINDOW WITH WITHOUT WORK WRAPPER WRITE
  
! 	XML_P XMLATTRIBUTES XMLCONCAT XMLELEMENT XMLEXISTS XMLFOREST XMLPARSE
  	XMLPI XMLROOT XMLSERIALIZE
  
  	YEAR_P YES_P
***
*** 9806,9811  func_expr:	func_name '(' ')' over_clause
--- 9807,9828 
  {
  	$$ = makeXmlExpr(IS_XMLELEMENT, $4, $6, $8,

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

2010-06-27 Thread Robert Haas 
On Sun, Jun 27, 2010 at 12:04 PM, Mike Fowler m...@mlfowler.com wrote:
 Thanks again for your help Robert, turns out the fault was in the pg_proc
 entry (the 3 up there should've been a two!). Once I took the grammar out it
 was quickly obvious where I'd gone wrong.

Glad it was a helpful suggestion.

 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).

So if you don't specify the xml_value, what does the xpath_expression
get applied to?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2010-06-27 Thread Mike Fowler 

Robert Haas wrote:

On Sun, Jun 27, 2010 at 12:04 PM, Mike Fowler m...@mlfowler.com wrote:
  

Thanks again for your help Robert, turns out the fault was in the pg_proc
entry (the 3 up there should've been a two!). Once I took the grammar out it
was quickly obvious where I'd gone wrong.



Glad it was a helpful suggestion.

  

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).



So if you don't specify the xml_value, what does the xpath_expression
get applied to?
  
From what I can gather the xpath_expression would be evalutated against 
an empty document thereby returning false for every xpath_expression 
except for 'true()'. Apache Derby has made the xml_value mandatory as 
well (though I'll stress my conclusion wasn't based on this fact). If 
you think it would better to adhere more closely to the standard I can 
certainly look to do so. From a cursory glance at libxml's API I think 
it should be straight forward to query against an empty document such 
that I wouldn't need ot code for the exceptional case (or cases if I've 
missed others).


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