Re: [PATCH] Re: [HACKERS] Adding XMLEXISTS to the grammar
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
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;
]]>
!
XML Predicates
IS DOCUMENT
--- 8554,8570
]]>
+
!
XML Predicates
+
+
+ XML Predicates
+
+
+
+ IS DOCUMENT
IS DOCUMENT
***
*** 8574,8579 SELECT xmlagg(x) FROM (SELECT * FROM test ORDER BY y DESC) AS tab;
--- 8582,8619
between documents and content fragments.
+
+
+ XMLEXISTS
+
+
+ XMLEXISTS
+
+
+
+ XMLEXISTS(xpath PASSING BY REF xml BY REF)
+
+
+
+ The function xmlexists returns true if the xpath
+ returns any nodes and false otherwise. If no xml is passed, the function
+ will return false as a XPath cannot be evaluated without content. See the
+ http://www.w3.org/TR/xpath/#section-Introduction";>W3C recommendation 'XML Path Language'
+ for a detailed explanation of why.
+
+
+
+ Example:
+
+
+
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***
*** 423,431 static TypeName *TableFuncTypeName(List *columns);
%type opt_check_option
%type xml_attribute_el
! %type xml_attribute_list xml_attributes
%type xml_root_version opt_xml_root_standalone
! %type document_or_content
%type xml_whitespace_option
%type common_table_expr
--- 423,432
%type opt_check_option
%type xml_attribute_el
! %type xml_attribute_list xml_attributes xmlexists_list
%type xml_root_version opt_xml_root_standalone
! %type xmlexists_query_argument_list
! %type document_or_content
%type xml_whitespace_option
%type 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, @1);
}
+ | XMLEXISTS '(' xmlexists_list ')'
+ {
+ /* xmlexists(A [PASSING BY REF B [BY REF]]) is converted to
+ * xmlexists(A, B)*/
+
+ FuncCall *n = makeNode(FuncCall);
+ n->funcname = SystemFuncName("xmlexists");
+
Re: [PATCH] Re: [HACKERS] 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 -- 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
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
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;
]]>
!
XML Predicates
IS DOCUMENT
--- 8554,8570
]]>
+
!
XML Predicates
+
+
+ XML Predicates
+
+
+
+ IS DOCUMENT
IS DOCUMENT
***
*** 8574,8579 SELECT xmlagg(x) FROM (SELECT * FROM test ORDER BY y DESC) AS tab;
--- 8582,8616
between documents and content fragments.
+
+
+ XMLEXISTS
+
+
+ XMLEXISTS
+
+
+
+ XMLEXISTS(xpath PASSING BY REF xml BY REF)
+
+
+
+ The function xmlexists returns true if the xml
+ satisfies the xpath and false otherwise.
+
+
+
+ Example:
+
+
+
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***
*** 423,431 static TypeName *TableFuncTypeName(List *columns);
%type opt_check_option
%type xml_attribute_el
! %type xml_attribute_list xml_attributes
%type xml_root_version opt_xml_root_standalone
! %type document_or_content
%type xml_whitespace_option
%type common_table_expr
--- 423,432
%type opt_check_option
%type xml_attribute_el
! %type xml_attribute_list xml_attributes xmlexists_list
%type xml_root_version opt_xml_root_standalone
! %type xmlexists_query_argument_list xml_default_passing_mechanism xml_passing_mechanism
! %type document_or_content
%type xml_whitespace_option
%type 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, @1);
}
+ | XMLEXISTS '(' xmlexists_list ')'
+ {
+ /* xmlexists(A [PASSING BY REF B [BY REF]]) is converted to
+ * xmlexists(A, B)*/
+
+ FuncCall *n = makeNode(FuncCall);
+ n->funcname = SystemFuncName("xmlexists");
+ n->args = $3;
+ n->agg_order = NIL;
+ n->agg_star = FALSE;
+ n->agg_distinct = FALSE;
+ n->func_variadic = FALSE;
+ n->over = NULL;
+ n->location = @1;
+ $$ = (Node *)n;
+ }
| XMLFOREST '(' xml_attribute_list ')'
{
$$ = makeXmlExpr(IS_XMLFOREST, NULL, $3, NIL, @1);
***
*** 9896,9901 xml_whitespace_option: PRESERVE WHITESPACE_P { $$ = TRUE; }
--- 9913,9946
| /
Re: [PATCH] Re: [HACKERS] Adding XMLEXISTS to the grammar
Robert Haas wrote: On Sun, Jun 27, 2010 at 12:04 PM, 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. 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
Re: [PATCH] Re: [HACKERS] Adding XMLEXISTS to the grammar
On Sun, Jun 27, 2010 at 12:04 PM, 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. 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
[PATCH] Re: [HACKERS] Adding XMLEXISTS to the grammar
and finally in pg_proc.h I have:
DATA(insert OID = 3037 ( xmlexists PGNSP PGUID 12 1 0 0 f f f t f i 3 0
16 "25 142" _null_ _null_ _null_ _null_ xml_exists _null_ _null_ _null_ ));
DESCR("evaluate XPath expression in a boolean context");
It looks like the pg_proc entry is creating an SQL function called
xmlexists referencing a C function called xml_exists, and the gram.y
changes want there to be an SQL function called xml_exists. I think
you should rip out all the catalog and parser changes for starters,
and just try to get it working as a regular old function. Once you
have that working, you can add the syntax support back in. I'd
suggest making the C and SQL function names the same as each other,
but different from the keyword you're planning to use (xmlexists).
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).
--
Mike Fowler
Registered Linux user: 379787
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***
*** 423,431 static TypeName *TableFuncTypeName(List *columns);
%type opt_check_option
%type xml_attribute_el
! %type xml_attribute_list xml_attributes
%type xml_root_version opt_xml_root_standalone
! %type document_or_content
%type xml_whitespace_option
%type common_table_expr
--- 423,432
%type opt_check_option
%type xml_attribute_el
! %type xml_attribute_list xml_attributes xmlexists_list
%type xml_root_version opt_xml_root_standalone
! %type xmlexists_query_argument_list xml_default_passing_mechanism xml_passing_mechanism
! %type document_or_content
%type xml_whitespace_option
%type 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, @1);
}
+ | XMLEXISTS '(' xmlexists_list ')'
+ {
+ /* xmlexists(A [PASSING BY REF B [BY REF]]) is converted to
+ * xmlexists(A, B)*/
+
+ FuncCall *n = makeNode(FuncCall);
+ n->funcname = SystemFuncName("xmlexists");
+ n->args = $3;
+ n->agg_order = NIL;
+ n->agg_star = FALSE;
+ n->agg_distinct = FALSE;
+ n->func_variadic = FALSE;
+ n->over = NULL;
+ n->location = @1;
+ $$ = (Node *)n;
+ }
| XMLFOREST '(' xml_attribute_list ')'
{
$$ = makeXmlExpr(IS_XMLFOREST, NULL, $3, NIL, @1);
***
*** 9896,9901 xml_whitespace_option: PRESERVE WHITESPACE_P { $$ = TRUE; }
--- 9913,9946
| /*EMPTY*/{ $$ = FALSE; }
;
+ xmlexists_list:
+ AexprConst xmlexists_query_argument_list
+ {
+ $$ = list_make2(makeTypeCast($1,SystemTypeName("text"), -1), $2);
+ }
+ ;
+
+ xmlexists_query_argument_list:
+ xml_default_passing_mechanism c_expr
+ {
+ $$ = $2;
+
