Mailing-List: contact derby-commits-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Derby Development" <derby-dev@db.apache.org>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r661204 - in /db/derby/code/trunk/java:
 engine/org/apache/derby/impl/sql/execute/MaxMinAggregator.java
 testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByTest.java
Date: Thu, 29 May 2008 04:20:51 -0000
To: derby-commits@db.apache.org
From: bpendleton@apache.org
Message-Id: <20080529042051.ED38F2388A0F@eris.apache.org>

Author: bpendleton
Date: Wed May 28 21:20:48 2008
New Revision: 661204

URL: http://svn.apache.org/viewvc?rev=661204&view=rev
Log:
DERBY-3219: GROUP BY query fails with ERROR XSDA7

The underlying cause of this problem is that the externalized data format
for a MaxMinAggregator instance includes the max (or min) value that the
aggregator is processing, and this data value happens to be embedded
*inside* of the overall externalized data format. However, the externalized
format for a SQLChar-based data value can use a "stream" format, in which
the explicit length of the value is not encoded, and rather the value is
read until an EOF is received, which means that such a value always has
to be the *last* value in the particular stream, and cannot be embedded
inside of a larger data structure.

In the case in question, the value was a string of length 0, which when
externalized looks like a streamed value, but can be distinguished
because the EOF exception occurs before any data has been read. But when
the value is included inside the larger MaxMinAggregator value, the EOF
exception does *not* occur immediately, but rather after the code in
SQLChar.readExternal has read beyond its own section, and has erroneously
consumed the other values from the MaxMinAggregator's external data.

The solution is to re-order the external format of the MaxMinAggregator,
such that the data value is the last item in the external data, by
calling super.writeExternal and super.readExternal *after* processing
the other MaxMinAggregator data. Since MaxMinAggregator instances are
never stored persistently in permanent data structures, but only in
temporary data structures such as overflow tables and sort buffers, this
should not cause any compatibility problems.

An alternative implementation, which involved changing SQLChar.writeExternal
to use an explicitly-delimited external format for a string of length
zero, rather than the streaming-until-EOF format, was rejected because it
would have increased the on-disk format of such values, and because it
could have caused compatibility problems by changing the on-disk format
of existing values.

The unit test for this bug fix involves the use of some SanityManager code
in the sorter, and hence is only effective when run in a sane debug build.


Modified:
    db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/execute/MaxMinAggregator.java
    db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByTest.java

Modified: db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/execute/MaxMinAggregator.java
URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/execute/MaxMinAggregator.java?rev=661204&r1=661203&r2=661204&view=diff
==============================================================================
--- db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/execute/MaxMinAggregator.java (original)
+++ db/derby/code/trunk/java/engine/org/apache/derby/impl/sql/execute/MaxMinAggregator.java Wed May 28 21:20:48 2008
@@ -88,11 +88,18 @@
 	// 
 	// FORMATABLE INTERFACE
 	// 
+	// Formatable implementations usually invoke the super()
+	// version of readExternal or writeExternal first, then
+	// do the additional actions here. However, since the
+	// superclass of this class requires that its externalized
+	// data must be the last data in the external stream, we
+	// invoke the superclass's read/writeExternal method
+	// last, not first. See DERBY-3219 for more discussion.
 	/////////////////////////////////////////////////////////////
 	public void writeExternal(ObjectOutput out) throws IOException
 	{
-		super.writeExternal(out);
 		out.writeBoolean(isMax);
+		super.writeExternal(out);
 	}
 
 	/** 
@@ -103,8 +110,8 @@
 	 */
 	public void readExternal(ObjectInput in) 
 		throws IOException, ClassNotFoundException {
-		super.readExternal(in);
 		isMax = in.readBoolean();
+		super.readExternal(in);
 	}
 	/**
 	 * Get the formatID which corresponds to this class.

Modified: db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByTest.java
URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByTest.java?rev=661204&r1=661203&r2=661204&view=diff
==============================================================================
--- db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByTest.java (original)
+++ db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/lang/GroupByTest.java Wed May 28 21:20:48 2008
@@ -29,6 +29,8 @@
 import junit.framework.Test;
 import junit.framework.TestSuite;
 
+import org.apache.derby.shared.common.sanity.SanityManager;
+
 import org.apache.derbyTesting.junit.BaseJDBCTestCase;
 import org.apache.derbyTesting.junit.CleanDatabaseTestSetup;
 import org.apache.derbyTesting.junit.JDBC;
@@ -95,6 +97,8 @@
         st.executeUpdate("insert into d2085 values (1,1,1,1), (1,2,3,4), " +
                 "(4,3,2,1), (2,2,2,2)");
 
+        st.execute("create table d3219 (a varchar(10), b varchar(1000))");
+
         st.executeUpdate("create table d2457_o (name varchar(20), ord int)");
         st.executeUpdate("create table d2457_a (ord int, amount int)");
         st.executeUpdate("insert into d2457_o values ('John', 1)," +
@@ -1596,6 +1600,69 @@
     }
 
     /**
+     * DERBY-3219: Check that MaxMinAggregator's external format works
+     * properly with a string of length 0.
+     */
+    public void testGroupByMaxWithEmptyString() throws SQLException
+    {
+        // Force all sorts to be external sorts, for DERBY-3219. This
+        // property only takes effect for debug sane builds, but that
+        // should be adequate since at least some developers routinely
+        // run tests in that configuration.
+        boolean wasSet = false;
+        if (SanityManager.DEBUG)
+        {
+            wasSet = SanityManager.DEBUG_ON("testSort");
+            SanityManager.DEBUG_SET("testSort");
+        }
+            
+        Statement st = createStatement();
+        loadRows();
+        ResultSet rs = st.executeQuery("select b,max(a) from d3219 group by b");
+        while (rs.next());
+        // If we can read the results, the test passed. DERBY-3219 resulted
+        // in an externalization data failure during the group by processing.
+        if (SanityManager.DEBUG)
+        {
+            if (! wasSet)
+                SanityManager.DEBUG_CLEAR("testSort");
+        }
+    }
+
+    /**
+     * Load enough rows into the table to get some externalized
+     * MaxMinAggregator instances. To ensure that the values are externalized,
+     * we set up this test to run with -Dderby.debug.true=testSort. Note that
+     * we load the column 'a' with a string of length 0, because DERBY-3219
+     * occurs when the computed MAX value is a string of length 0.
+     */
+    private void loadRows()
+        throws SQLException
+    {
+        PreparedStatement ps = prepareStatement(
+                "insert into d3219 (a, b) values ('', ?)");
+
+        for (int i = 0; i < 2000; i++)
+        {
+            ps.setString(1, genString(1000));
+            ps.executeUpdate();
+        }
+    }
+    private static String genString(int len)
+    {
+        StringBuffer buf = new StringBuffer(len);
+
+        for (int i = 0; i < len; i++)
+            buf.append(chars[(int) (chars.length * Math.random())]);
+        return buf.toString();
+    }
+    private static char []chars = {
+        'q','w','e','r','t','y','u','i','o','p',
+        'a','s','d','f','g','h','j','k','l',
+        'z','x','c','v','b','n','m'
+    };
+
+    /**
       * DERBY-2457: Derby does not support column aliases in the
       * GROUP BY and HAVING clauses.
       */