Hi 2018-01-23 8:13 GMT+01:00 Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp >:
> Hello, I returned to this. > > I thouroughly checked the translator's behavior against the XPath > specifications and checkd out the documentation and regression > test. Almost everything is fine for me and this would be the last > comment from me. > > At Fri, 24 Nov 2017 18:32:43 +0100, Pavel Stehule <pavel.steh...@gmail.com> > wrote in <CAFj8pRB7Fs_2DrtUTGhTmQb+KReXPH6SG62hGWO3KVL_eZYCaA@ > mail.gmail.com> > > 2017-11-24 18:13 GMT+01:00 Pavel Stehule <pavel.steh...@gmail.com>: > > > > > > > > > > > 2017-11-24 17:53 GMT+01:00 Pavel Stehule <pavel.steh...@gmail.com>: > > > > > >> Hi > > >> > > >> 2017-11-22 22:49 GMT+01:00 Thomas Munro < > thomas.mu...@enterprisedb.com>: > > >> > > >>> On Thu, Nov 9, 2017 at 10:11 PM, Pavel Stehule < > pavel.steh...@gmail.com> > > >>> wrote: > > >>> > Attached new version. > > >>> > > >>> Hi Pavel, > > >>> > > >>> FYI my patch testing robot says[1]: > > >>> > > >>> xml ... FAILED > > >>> > > >>> regression.diffs says: > > >>> > > >>> + SELECT x.* FROM t1, xmltable(XMLNAMESPACES(DEFAULT 'http://x.y'), > > >>> '/rows/row' PASSING t1.doc COLUMNS data int PATH > > >>> 'child::a[1][attribute::hoge="haha"]') as x; > > >>> + data > > >>> + ------ > > >>> + (0 rows) > > >>> + > > >>> > > >>> Maybe you forgot to git-add the expected file? > > >>> > > >>> [1] https://travis-ci.org/postgresql-cfbot/postgresql/ > builds/305979133 > > >> > > >> > > >> unfortunately xml.out has 3 versions and is possible so one version > > >> should be taken elsewhere than my comp. > > >> > > >> please can me send your result xml.out file? > > >> > > > > > > looks like this case is without xml support so I can fix on my comp. > > > > > > > > fixed regress test > > (I wouldn't have found that..) > > I have three comments on the behavior and one on documentation. > > 1. Lack of syntax handling. > > ["'" [^'] "'"] is also a string literal, but getXPathToken is > forgetting that and applying default namespace mistakenly to the > astring content. > > 2. Additional comment might be good. > > It might be better having additional description about default > namespace in the comment starts from "Namespace mappings are > passed as text[]" in xpth_internal(). > > 3. Inconsistent behavior from named namespace. > > | - function context, aliases are <emphasis>local</emphasis>). > | + function context, aliases are <emphasis>local</emphasis>). Default > namespace has > | + empty name (empty string) and should be only one. > > This works as the description, on the other hand the same > namespace prefix can be defined twice or more in the array and > the last one is in effect. I don't see a reason for > differenciating the default namespace case. > > > 4. Comments on the documentation part. > > # Even though I'm not sutable for commenting on wording... > > | + Inside predicate literals <literal>and</literal>, > <literal>or</literal>, > | + <literal>div</literal> and <literal>mod</literal> are used as > keywords > | + (XPath operators) every time and default namespace are not applied > there. > > *I*'d like to have a comma between the predicate and literals, > and have a 'a' before prediate. Or 'Literals .. inside a > predicate' might be better? > > 'are used as keywords' might be better being 'are identifed as > keywords'? > > Default namespace is applied to tag names except the listed > keywords even inside a predicate. So 'are not applied there' > might be better being 'are not applied to them'? Or 'are not > applied in the case'? > > | + If you would to use these literals like tag names, then the > default namespace > | + should not be used, and these literals should be explicitly > | + labeled. > | + </para> > > Default namespace is not applied *only to* such keywords inside a > predicate. Even if an Xpath expression contains such a tag name, > default namespace still works for other tags. Does the following > make sense? > > + Use named namespace to qualify such tag names appear in an > + XPath predicate. > > please, can you append examples of mentioned issues. I'll fix it faster. Thank you very much Pavel > > > === > After the aboves are addressed (even or rejected), I think I > don't have no additional comment. > > > - This current patch applies safely (with small shifts) on the > current master. > > - The code looks fine for me. > > - This patch translates the given XPath expression by prefixing > unprefixed tag names with a special namespace prefix only in > the case where default namespace is defined, so the existing > behavior is not affected. > > - The syntax is existing but just not implemented so I don't > think no arguemnts needed here. > > - It undocumentedly inhibits the usage of the namespace prefix > "pgdefnamespace.pgsqlxml.internal" but I believe no one can > notice that. > > - The default-ns translator (xpath_parser.c) seems working > perfectly with some harmless exceptions. > > (xpath specifications is here: https://www.w3.org/TR/1999/ > REC-xpath-19991116/) > > Related unused features (and not documented?): > context variables ($n notations), > user-defined functions (or function names prefixed by a namespace > prefix) > > Newly documented behavior: > the default namespace isn't applied to and/or/div/mod. > > - Dodumentation looks enough. > > - Regression test doesn't cover the XPath syntax but it's not > viable. I am fine with the basic test cases added by the > current patch. > > regards, > > -- > Kyotaro Horiguchi > NTT Open Source Software Center > >
