You are viewing a plain text version of this content. The canonical link for it is here.
Posted to xindice-dev@xml.apache.org by Todd Byrne <by...@cns.montana.edu> on 2005/05/20 20:18:11 UTC
[PATCH] org/apache/xindice/core/xupdate/XUpdateImpl.java
Attached is a patch that fixes several issues with collection level
XUpdate commands. This proposed patch fixes bug 25892 and issues with
accidently trying to running XUpdate variables as XPaths.
The one thing I am not sure about is I wait until all of the commands
are executed on all of the documents in the collection before committing
the changes back. This could be potentially hard on memory but it
guarantees that if the one of the commands blew up it doesn't leave the
database in a bad state.
Comments and suggestions welcome.
Todd Byrne
Re: [PATCH] org/apache/xindice/core/xupdate/XUpdateImpl.java
Posted by by...@cns.montana.edu.
On Fri, 20 May 2005, Vadim Gritsenko wrote:
> Todd Byrne wrote:
> > Attached is a patch that fixes several issues with collection level
> > XUpdate commands. This proposed patch fixes bug 25892 and issues with
> > accidently trying to running XUpdate variables as XPaths.
> >
> > The one thing I am not sure about is I wait until all of the commands
> > are executed on all of the documents in the collection before committing
> > the changes back. This could be potentially hard on memory but it
> > guarantees that if the one of the commands blew up it doesn't leave the
> > database in a bad state.
>
> Yep - same here - I have same memory concern as you do.
>
> Patch looks good, and if you don't mind adjusting it, please re-send with these
> minor tweaks:
>
> * HashMap is more appropriate here.
> Hashtable is synchronized, which is not needed here.
> And switch to map.entrySet().iterator() instead of Enumeration.
Will do.
>
> * Please add a TODO comment before docsUpdated declaration,
> something to the effect of "don't buffer changed documents
> in the memory if underlying collection supports transactions"
> (idea is that if in future collection supports transactions,
> no in memory document map will be needed)
Real transactions would be nice so a TODO makes sense here.
>
> * Can you add a test case for this scenario, so that
> we can automatically check for regressions in the future?
> Probably it can be added into the XUpdateQueryTest.
I have a test case up. I will look at the XUpdateQueryTest and build up a
patch for that file also.
>
> * Don't use tabs!!! :-) 4 space indenting only, please.
Oops. :)
>
>
> And, as I am not huge xupdate user myself, can you clarify what this condition
> is needed for:
>
>
> > + if (selector != null && !selector.startsWith("$")) {
>
>
> (I mean, 'selector.startsWith("$")' part)
All variables in XUpdate start with a '$'. so you can have a
select="$myVar" which can be a prestored set of nodes.
I will make the changes and resubmit.
Todd
>
>
> Thanks,
> Vadim
>
>
> >
> > Comments and suggestions welcome.
> >
> > Todd Byrne
> >
> >
> > ------------------------------------------------------------------------
> >
> > --- xml-xindice/java/src/org/apache/xindice/core/xupdate/XUpdateImpl.java 2004-02-07 19:50:54.000000000 -0700
> > +++ /home/byrne/jbproject/xindice-cvs-src/xml-xindice/java/src/org/apache/xindice/core/xupdate/XUpdateImpl.java 2004-08-30 10:30:39.000000000 -0600
> > @@ -24,6 +24,7 @@
> > import org.apache.xindice.xml.NodeSource;
> > import org.apache.xindice.xml.dom.CompressedNode;
> > import org.apache.xindice.xml.dom.DBNode;
> > +import org.apache.xindice.core.data.Key;
> >
> > import org.w3c.dom.Document;
> > import org.w3c.dom.Node;
> > @@ -150,6 +151,7 @@
> > public void execute(Collection col) throws Exception {
> >
> > int attribIndex = 0;
> > + Hashtable docsUpdated = new Hashtable();
> > for (int i = 0; i < super.query[0].size(); i++) {
> > int cmdID = ((Integer) super.query[0].elementAt(i)).intValue();
> >
> > @@ -159,29 +161,32 @@
> > String selector = (String) attribs.get("select");
> >
> > // If we found an XPath selector we need to execute the commands.
> > - if (selector != null) {
> > + if (selector != null && !selector.startsWith("$")) {
> > NodeSet ns = col.queryCollection("XPath", selector, nsMap);
> > Document lastDoc = null;
> > while (ns != null && ns.hasMoreNodes()) {
> > DBNode node = (DBNode) ns.getNextNode();
> > Document doc = node.getOwnerDocument();
> > + NodeSource source = node.getSource();
> > + Node contextNode = doc.getDocumentElement();
> >
> > - if (doc == lastDoc) {
> > + if (docsUpdated.containsKey(source.getKey())) {
> > continue; // We only have to process it once
> > } else {
> > - lastDoc = doc;
> > + docsUpdated.put(source.getKey(), doc);
> > }
> > -
> > - NodeSource source = node.getSource();
> > -
> > - Node contextNode = doc.getDocumentElement();
> > execute(contextNode);
> > -
> > - col.setDocument(source.getKey(), doc);
> > }
> > }
> > }
> > }
> > + // update all documents at once
> > + // this way we don't get any half run xupdate commands.
> > + Enumeration keys = docsUpdated.keys();
> > + while(keys.hasMoreElements()) {
> > + Key docID = (Key)keys.nextElement();
> > + col.setDocument(docID, (Document)docsUpdated.get(docID));
> > + }
> > }
> >
> > public int getModifiedCount() {
>
Re: [PATCH] org/apache/xindice/core/xupdate/XUpdateImpl.java
Posted by Vadim Gritsenko <va...@reverycodes.com>.
Todd Byrne wrote:
> Attached is a patch that fixes several issues with collection level
> XUpdate commands. This proposed patch fixes bug 25892 and issues with
> accidently trying to running XUpdate variables as XPaths.
>
> The one thing I am not sure about is I wait until all of the commands
> are executed on all of the documents in the collection before committing
> the changes back. This could be potentially hard on memory but it
> guarantees that if the one of the commands blew up it doesn't leave the
> database in a bad state.
Yep - same here - I have same memory concern as you do.
Patch looks good, and if you don't mind adjusting it, please re-send with these
minor tweaks:
* HashMap is more appropriate here.
Hashtable is synchronized, which is not needed here.
And switch to map.entrySet().iterator() instead of Enumeration.
* Please add a TODO comment before docsUpdated declaration,
something to the effect of "don't buffer changed documents
in the memory if underlying collection supports transactions"
(idea is that if in future collection supports transactions,
no in memory document map will be needed)
* Can you add a test case for this scenario, so that
we can automatically check for regressions in the future?
Probably it can be added into the XUpdateQueryTest.
* Don't use tabs!!! :-) 4 space indenting only, please.
And, as I am not huge xupdate user myself, can you clarify what this condition
is needed for:
> + if (selector != null && !selector.startsWith("$")) {
(I mean, 'selector.startsWith("$")' part)
Thanks,
Vadim
>
> Comments and suggestions welcome.
>
> Todd Byrne
>
>
> ------------------------------------------------------------------------
>
> --- xml-xindice/java/src/org/apache/xindice/core/xupdate/XUpdateImpl.java 2004-02-07 19:50:54.000000000 -0700
> +++ /home/byrne/jbproject/xindice-cvs-src/xml-xindice/java/src/org/apache/xindice/core/xupdate/XUpdateImpl.java 2004-08-30 10:30:39.000000000 -0600
> @@ -24,6 +24,7 @@
> import org.apache.xindice.xml.NodeSource;
> import org.apache.xindice.xml.dom.CompressedNode;
> import org.apache.xindice.xml.dom.DBNode;
> +import org.apache.xindice.core.data.Key;
>
> import org.w3c.dom.Document;
> import org.w3c.dom.Node;
> @@ -150,6 +151,7 @@
> public void execute(Collection col) throws Exception {
>
> int attribIndex = 0;
> + Hashtable docsUpdated = new Hashtable();
> for (int i = 0; i < super.query[0].size(); i++) {
> int cmdID = ((Integer) super.query[0].elementAt(i)).intValue();
>
> @@ -159,29 +161,32 @@
> String selector = (String) attribs.get("select");
>
> // If we found an XPath selector we need to execute the commands.
> - if (selector != null) {
> + if (selector != null && !selector.startsWith("$")) {
> NodeSet ns = col.queryCollection("XPath", selector, nsMap);
> Document lastDoc = null;
> while (ns != null && ns.hasMoreNodes()) {
> DBNode node = (DBNode) ns.getNextNode();
> Document doc = node.getOwnerDocument();
> + NodeSource source = node.getSource();
> + Node contextNode = doc.getDocumentElement();
>
> - if (doc == lastDoc) {
> + if (docsUpdated.containsKey(source.getKey())) {
> continue; // We only have to process it once
> } else {
> - lastDoc = doc;
> + docsUpdated.put(source.getKey(), doc);
> }
> -
> - NodeSource source = node.getSource();
> -
> - Node contextNode = doc.getDocumentElement();
> execute(contextNode);
> -
> - col.setDocument(source.getKey(), doc);
> }
> }
> }
> }
> + // update all documents at once
> + // this way we don't get any half run xupdate commands.
> + Enumeration keys = docsUpdated.keys();
> + while(keys.hasMoreElements()) {
> + Key docID = (Key)keys.nextElement();
> + col.setDocument(docID, (Document)docsUpdated.get(docID));
> + }
> }
>
> public int getModifiedCount() {