Bug #3281
closed
Getting wrong result when using streaming with for-each-group group-by and map variable inside to collect some data from current group
100%
Description
I am trying to test whether the grouping examples in the XSLT 3.0 spec can be converted to use streaming, I have managed to convert an example so that is passes the streamability analyis in Saxon 9.8 EE, however, unfortunately the result I then get is wrong and different to running the code without streaming.
Here are the details, the input sample is
<?xml version="1.0" encoding="UTF-8"?>
<cities>
<city name="Milano" country="Italia" pop="5"/>
<city name="Paris" country="France" pop="7"/>
<city name="München" country="Deutschland" pop="4"/>
<city name="Lyon" country="France" pop="2"/>
<city name="Venezia" country="Italia" pop="1"/>
</cities>
to convert the original code
<xsl:for-each-group select="cities/city" group-by="@country">
<tr>
<td><xsl:value-of select="position()"/></td>
<td><xsl:value-of select="current-grouping-key()"/></td>
<td>
<xsl:for-each select="current-group()/@name">
<xsl:sort select="."/>
<xsl:if test="position() ne 1">, </xsl:if>
<xsl:value-of select="."/>
</xsl:for-each>
</td>
<td><xsl:value-of select="sum(current-group()/@pop)"/></td>
</tr>
</xsl:for-each-group>
to a streamable example I have removed the sort and then tried to use a map to store the result of the list of name attributes and of the sum:
<?xml version="1.0" encoding="UTF-8"?>
<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
xmlns:xs="http://www.w3.org/2001/XMLSchema"
xmlns:math="http://www.w3.org/2005/xpath-functions/math" exclude-result-prefixes="xs math"
version="3.0">
<xsl:param name="STREAMABLE" static="yes" as="xs:boolean" select="true()"/>
<xsl:mode _streamable="{$STREAMABLE}"/>
<xsl:output method="html" indent="yes"/>
<xsl:template match="/">
<html>
<head>
<title>Population grouped by country</title>
</head>
<body>
<xsl:apply-templates/>
</body>
</html>
</xsl:template>
<xsl:template match="cities">
<table>
<tr>
<th>Position</th>
<th>Country</th>
<th>City List</th>
<th>Population</th>
</tr>
<xsl:fork>
<xsl:for-each-group select="city" group-by="@country">
<tr>
<td>
<xsl:value-of select="position()"/>
</td>
<td>
<xsl:value-of select="current-grouping-key()"/>
</td>
<xsl:variable name="data"
select="map { 'cities' : current-group()/@name/string(), 'pop-sum' : sum(current-group()/@pop) }"/>
<td>
<xsl:value-of select="$data?cities" separator=", "/>
</td>
<td>
<xsl:value-of select="$data?pop-sum"/>
</td>
</tr>
</xsl:for-each-group>
</xsl:fork>
</table>
</xsl:template>
</xsl:stylesheet>
When I run that with Saxon-EE 9.8.0.1J from the command line I get the following odd result, with wrong data and various duplicated td elements:
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<title>Population grouped by country</title>
</head>
<body>
<table>
<tr>
<th>Position</th>
<th>Country</th>
<th>City List</th>
<th>Population</th>
</tr>
<tr>
<td>1</td>
<td>Italia</td>
<td>1</td>
<td>Italia</td>
<td>1</td>
<td>Italia</td>
<td>Milano</td>
<td>5</td>
</tr>
<tr>
<td>2</td>
<td>France</td>
<td>2</td>
<td>France</td>
<td>2</td>
<td>France</td>
<td>Paris</td>
<td>7</td>
</tr>
<tr>
<td>3</td>
<td>Deutschland</td>
<td>3</td>
<td>Deutschland</td>
<td>3</td>
<td>Deutschland</td>
<td>München</td>
<td>4</td>
</tr>
</table>
</body>
</html>
When I turn of streaming by setting the parameter STREAMABLE=0 on the command line I get the correct, wanted result:
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<title>Population grouped by country</title>
</head>
<body>
<table>
<tr>
<th>Position</th>
<th>Country</th>
<th>City List</th>
<th>Population</th>
</tr>
<tr>
<td>1</td>
<td>Italia</td>
<td>Milano, Venezia</td>
<td>6</td>
</tr>
<tr>
<td>2</td>
<td>France</td>
<td>Paris, Lyon</td>
<td>9</td>
</tr>
<tr>
<td>3</td>
<td>Deutschland</td>
<td>München</td>
<td>4</td>
</tr>
</table>
</body>
</html>
9.7 EE when run with streaming also gives a wrong result, it does not output duplicated td elements but the data in the table cells coming from the map is wrong:
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<title>Population grouped by country</title>
</head>
<body>
<table>
<tr>
<th>Position</th>
<th>Country</th>
<th>City List</th>
<th>Population</th>
</tr>
<tr>
<td>1</td>
<td>Italia</td>
<td>Milano</td>
<td>0</td>
</tr>
<tr>
<td>2</td>
<td>France</td>
<td>Paris</td>
<td>0</td>
</tr>
<tr>
<td>3</td>
<td>Deutschland</td>
<td>München</td>
<td>0</td>
</tr>
</table>
</body>
</html>
I have also tried a different approach to using a map by nesting another @xsl:fork@, unfortunately, that too does not give the right result when using streaming with Saxon 9.8. I will post that later as a separate issue.
Updated by Michael Kay almost 7 years ago
- Status changed from New to In Progress
- Assignee set to Michael Kay
Created XSLT30 test case si-fork-803; problem reproduced.
Updated by Michael Kay almost 7 years ago
It's clear that streamed grouping still has some way to go before the more complex cases work properly... There's a lot going on here and it's not at all easy to disentangle the pieces.
For example, the two map entries are being handled quite differently. The sum(cg()/@pop) is being handled during the startElement() event, while the @name/string() is being handled during the attribute() event.
If we take the @name/string() case, then when we hit the second startElement() in a group, the ForEachGroupParallelAction feed calls NewMapStreamer.startSelectedParentNode(), but because the child expressions (both map:entry() calls) are consuming, this does nothing.
When we hit startElement() for a city in a new group, we create a watch which triggers on "." and feeds into a NewMapForkingFeed. We then call open() on this NewMapForkingFeed, This calls open() on the next feed in the pipeline, which first creates the tr start tag and td elements containing position() and current-grouping-key(). It then creates a map (HTM2004) and processes the two child expressions, which are map-entry() calls for "cities" and "pop-sum" respectively. For each of these it creates and registers a Watch. The first Watch triggers on @name, and feeds into the NewMapForkingFeed; it then opens the watch, which calls open() on feeds further down the pipeline. The ComplexNodeEventFeed that creates the tr start tag does nothing because it recognizes that this is a second call on open(); but the BlockFeed has no similar logic, and therefore repeats the creation of the two initial td elements. We could add defensive code to BlockFeed to prevent open() doing anything on subsequent calls, or (better) we could try and fix NewMapForkingFeed.open() so it doesn't open() the rest of the pipeline twice.
I've made this change and we've now eliminated the excess td elements. But the map entries are wrong: they only contain data relating to the first item in the group.
Updated by Michael Kay almost 7 years ago
The problem now seems to be that ForEachGroupParallelAction.startSelectedParentNode() is calling NewMapForkingFeed.open() on the first element in a group, and this has the effect of creating the map and updating it to reflect the first element; but on subsequent elements in the group, no effective action is being taken.
NewMapForkingFeed.open() gets a watch, and adds this to the WatchManager, which immediately opens it. This calls ForEachAction.open(), which calls MapEntryFeed.open(), but this now does nothing. NewMapForkingFeed.open() then proceeds to process the child expressions of the map constructor, and for those that are consuming (which is both of them), it adds a watch to the WatchManager.
The first of these (Trigger+2034) feeds @pop (via type checking and atomizing feeds) to a FoldFeed for the sum() function, which in turn feeds into map:entry(). When this watch is added to the WatchManager it is immediately opened (which initializes a new map entry).
ForEachGroupParallelAction.startSelectedParentNode() then calls the groupFeed.startSelectedParentNode(), which does nothing because the relevant actions are consuming.
So we return to WatchManager.startElement(city) which continues the search for matching Watches, which finds Trigger+2034, which feeds the value of @pop into the FoldFeed for sum()
The next event is WatchManager.attribute(name) which triggers a Watch which feeds pretty directly into the MapEntryFeed for the "city" entry.
On WatchManager.endElement(city), we call close() on the FoldFeed for the sum() expression, which writes the sum so far to the map entry. This is clearly wrong: we should only close this fold at the end of the group. Having called close(), the WatchManager then removes the watch. The feed for the "cities" map entry is then closed and removed in the same way. Finally endSelectedParentNode() is called on the ForEachGroupParallelAction, but this does nothing.
So the issue seems to be that the current-group() watches are prematurely closed at the end of the first node in the group, and the obvious way of preventing this is to give them a different CLOSEDOWN_ACTION. However, this isn't straightforward, because the two watches are actually added by NewMapForkingFeed.open(), which has no idea that there is grouping going on.
There's a mysterious mechanism in the WatchManager that we could possibly exploit at this point, called the groupingScopeStack. If we add a watch when the groupingScopeStack is non-empty, then the watch is added to the top entry on this stack, with the effect that it will only be closed at the end of the group. Currently we are only adding an entry to this stack for partitioned grouping (group-adjacent, etc), and not for parallel grouping (group-by). It looks like we invented this mechanism to handle partitioned grouping, and we need to use it or extend it to handle parallel grouping as well.
Updated by Michael Kay almost 7 years ago
After some more experiments, this is the current situation.
After processing the first three cities (each of which starts a new group), we have 9 watches registered in the WatchManager: 2 for the outer xsl:apply-templates calls, one for the active xsl:for-each-group, and three groups of two: each new group has registered two watches for the two current-group() consuming expressions.
When we get to the fourth city, starting a new group, the for-each-group watch fires, but none of the current-group() watches fires, because they use a selection pattern that is tied to a specific anchor node, and the anchor node in question is the first node of the group.
So there are two issues: (a) we really don't want to be accumulating watches in the WatchManager in this way, especially if they can never fire; and (b) we need to find some way of changing the anchor node in the watches so they fire for the second and subsequent nodes in the group.
I have fixed (b): when we process the second and subsequent nodes in a group, we reset the anchor node for all watches connected to the group to the node currently being processed. The test now gives correct results.
We still have to worry about the inefficiency of having N watches present in the WatchManager where N is the number of groups. And of course we have to fix any regressions that these design changes cause to other tests.
Updated by Michael Kay almost 7 years ago
- Status changed from In Progress to Resolved
- Fix Committed on Branch 9.8 added
Now fixed, and all regression tests working. Fix applies to the 9.8 branch only - I'm not proposing to fix outstanding issues with streamed grouping in 9.7.
Updated by Michael Kay almost 7 years ago
- Status changed from Resolved to In Progress
Reopened: regression in sx-MapExpr tests.
Updated by Michael Kay almost 7 years ago
- Status changed from In Progress to Resolved
The regression has now been resolved.
Updated by O'Neil Delpratt almost 7 years ago
- Status changed from Resolved to Closed
- % Done changed from 0 to 100
- Fixed in Maintenance Release 9.8.0.2 added
Patch applied in the Saxon 9.8.0.2 maintenance release
Please register to edit this issue
.