You are viewing a plain text version of this content. The canonical link for it is here.
Posted to derby-commits@db.apache.org by ab...@apache.org on 2007/12/21 20:21:53 UTC
svn commit: r606277 - in /db/derby/code/branches/10.3/java:
engine/org/apache/derby/impl/sql/compile/
testing/org/apache/derbyTesting/functionTests/tests/lang/
Author: abrown
Date: Fri Dec 21 11:21:53 2007
New Revision: 606277
URL: http://svn.apache.org/viewvc?rev=606277&view=rev
Log:
DERBY-3253: Port changes to 10.3.
Modified:
db/derby/code/branches/10.3/java/engine/org/apache/derby/impl/sql/compile/BinaryRelationalOperatorNode.java
db/derby/code/branches/10.3/java/engine/org/apache/derby/impl/sql/compile/OrNode.java
db/derby/code/branches/10.3/java/testing/org/apache/derbyTesting/functionTests/tests/lang/InListMultiProbeTest.java
Modified: db/derby/code/branches/10.3/java/engine/org/apache/derby/impl/sql/compile/BinaryRelationalOperatorNode.java
URL: http://svn.apache.org/viewvc/db/derby/code/branches/10.3/java/engine/org/apache/derby/impl/sql/compile/BinaryRelationalOperatorNode.java?rev=606277&r1=606276&r2=606277&view=diff
==============================================================================
--- db/derby/code/branches/10.3/java/engine/org/apache/derby/impl/sql/compile/BinaryRelationalOperatorNode.java (original)
+++ db/derby/code/branches/10.3/java/engine/org/apache/derby/impl/sql/compile/BinaryRelationalOperatorNode.java Fri Dec 21 11:21:53 2007
@@ -80,9 +80,12 @@
* this BinRelOp is for an IN-list probe predicate; 2) if the
* optimizer chooses a plan for which the probe predicate is
* not usable as a start/stop key then we'll "revert" the pred
- * back to the InListOperatorNode referenced here.
+ * back to the InListOperatorNode referenced here. NOTE: Once
+ * set, this variable should *only* ever be accessed via the
+ * isInListProbeNode() or getInListOp() methods--see comments
+ * in the latter method for more.
*/
- InListOperatorNode inListProbeSource = null;
+ private InListOperatorNode inListProbeSource = null;
public void init(Object leftOperand, Object rightOperand)
{
@@ -153,10 +156,46 @@
* If this rel op was created for an IN-list probe predicate then return
* the underlying InListOperatorNode. Will return null if this rel
* op is a "legitimate" relational operator (as opposed to a disguised
- * IN-list).
+ * IN-list). With the exception of nullability checking via the
+ * isInListProbeNode() method, all access to this.inListProbeSource
+ * MUST come through this method, as this method ensures that the
+ * left operand of the inListProbeSource is set correctly before
+ * returning it.
*/
protected InListOperatorNode getInListOp()
{
+ if (inListProbeSource != null)
+ {
+ /* Depending on where this probe predicate currently sits
+ * in the query tree, this.leftOperand *may* have been
+ * transformed, replaced, or remapped one or more times
+ * since inListProbeSource was last referenced. Since the
+ * leftOperand of the IN list should be the same regardless
+ * of which "version" of the operation we're looking at
+ * (i.e. the "probe predicate" version (this node) vs the
+ * original version (inListProbeSource)), we have to make
+ * sure that all of the changes made to this.leftOperand
+ * are reflected in inListProbeSource's leftOperand, as
+ * well. In doing so we ensure the caller of this method
+ * will see an up-to-date version of the InListOperatorNode--
+ * and thus, if the caller references the InListOperatorNode's
+ * leftOperand, it will see the correct information. One
+ * notable example of this is at code generation time, where
+ * if this probe predicate is deemed "not useful", we'll
+ * generate the underlying InListOperatorNode instead of
+ * "this". For that to work correctly, the InListOperatorNode
+ * must have the correct leftOperand. DERBY-3253.
+ *
+ * That said, since this.leftOperand will always be "up-to-
+ * date" w.r.t. the current query tree (because this probe
+ * predicate sits in the query tree and so all relevant
+ * transformations will be applied here), the simplest way
+ * to ensure the underlying InListOperatorNode also has an
+ * up-to-date leftOperand is to set it to this.leftOperand.
+ */
+ inListProbeSource.setLeftOperand(this.leftOperand);
+ }
+
return inListProbeSource;
}
@@ -777,7 +816,7 @@
* the IN-list). That would lead to wrong results (missing rows)
* because that restriction is incorrect.
*/
- if (inListProbeSource != null)
+ if (isInListProbeNode())
return false;
FromTable ft;
@@ -1215,7 +1254,7 @@
* it a "relational operator"; it's actually a disguised IN-list
* operator.
*/
- return (inListProbeSource == null);
+ return !isInListProbeNode();
}
/** @see ValueNode#isBinaryEqualsOperatorNode */
@@ -1225,11 +1264,18 @@
* it as an "equals operator"; it's actually a disguised IN-list
* operator.
*/
- return (inListProbeSource == null) &&
+ return !isInListProbeNode() &&
(operatorType == RelationalOperator.EQUALS_RELOP);
}
- /** @see ValueNode#isInListProbeNode */
+ /**
+ * @see ValueNode#isInListProbeNode
+ *
+ * It's okay for this method to reference inListProbeSource directly
+ * because it does not rely on the contents of inListProbeSource's
+ * leftOperand, and a caller of this method cannot gain access to
+ * inListProbeSource's leftOperand through this method.
+ */
public boolean isInListProbeNode()
{
return (inListProbeSource != null);
@@ -1247,7 +1293,7 @@
/* If this rel op is for a probe predicate then we do not treat
* it as an equality node; it's actually a disguised IN-list node.
*/
- if (inListProbeSource != null)
+ if (isInListProbeNode())
return false;
ColumnReference cr = getColumnOperand(optTable,
Modified: db/derby/code/branches/10.3/java/engine/org/apache/derby/impl/sql/compile/OrNode.java
URL: http://svn.apache.org/viewvc/db/derby/code/branches/10.3/java/engine/org/apache/derby/impl/sql/compile/OrNode.java?rev=606277&r1=606276&r2=606277&view=diff
==============================================================================
--- db/derby/code/branches/10.3/java/engine/org/apache/derby/impl/sql/compile/OrNode.java (original)
+++ db/derby/code/branches/10.3/java/engine/org/apache/derby/impl/sql/compile/OrNode.java Fri Dec 21 11:21:53 2007
@@ -152,7 +152,7 @@
if (left instanceof BinaryRelationalOperatorNode)
{
bron = (BinaryRelationalOperatorNode)left;
- if (bron.getInListOp() == null)
+ if (!bron.isInListProbeNode())
{
SanityManager.THROWASSERT(
"isRelationalOperator() unexpectedly returned "
@@ -223,7 +223,7 @@
OrNode on = (OrNode) vn;
BinaryRelationalOperatorNode bron =
(BinaryRelationalOperatorNode) on.getLeftOperand();
- if (bron.getInListOp() != null)
+ if (bron.isInListProbeNode())
{
/* If we have an OR between multiple IN-lists on the same
* column then just combine them into a single IN-list.
Modified: db/derby/code/branches/10.3/java/testing/org/apache/derbyTesting/functionTests/tests/lang/InListMultiProbeTest.java
URL: http://svn.apache.org/viewvc/db/derby/code/branches/10.3/java/testing/org/apache/derbyTesting/functionTests/tests/lang/InListMultiProbeTest.java?rev=606277&r1=606276&r2=606277&view=diff
==============================================================================
--- db/derby/code/branches/10.3/java/testing/org/apache/derbyTesting/functionTests/tests/lang/InListMultiProbeTest.java (original)
+++ db/derby/code/branches/10.3/java/testing/org/apache/derbyTesting/functionTests/tests/lang/InListMultiProbeTest.java Fri Dec 21 11:21:53 2007
@@ -212,11 +212,10 @@
}
/**
- * The one test fixture for this test. Executes three different types
- * of queries ("strategies") repeatedly with an increasing number of
- * values in the IN list. Underneath we will check the query plan
- * for each query to make sure that Derby is doing multi-probing as
- * expected.
+ * Executes three different types of queries ("strategies") repeatedly
+ * with an increasing number of values in the IN list. Underneath we
+ * will check the query plan for each query to make sure that Derby is
+ * doing multi-probing as expected.
*/
public void testMultiProbing() throws Exception
{
@@ -495,6 +494,54 @@
st.execute("drop table mytable");
ps.close();
+ st.close();
+ }
+
+ /**
+ * Test the scenario in which Derby creates an IN-list probe
+ * predicate, remaps its left operand to point to a nested
+ * SELECT query, and then decides to *not* use the probe
+ * predicate in the final plan. The remapping of the left
+ * operand will cause the probe predicate's left operand to
+ * be set to a different ColumnReference object--one that
+ * points to the target table in the subselect. Then when
+ * the optimizer decides to *not* use the probe predicate
+ * in the final query, we'll revert back to the original IN
+ * list (InListOperatorNode) and generate that for the query.
+ * When we do so, the left operand of the InListOperatorNode
+ * must reflect the fact that the IN operation's left operand
+ * has changed (it now points to the table from the subselect).
+ * Otherwise the InListOperatorNode will generate an invalid
+ * ColumnReference. DERBY-3253.
+ */
+ public void testProbePredPushedIntoSelectThenReverted()
+ throws Exception
+ {
+ Statement st = createStatement();
+
+ st.execute("create table d3253 (i int, vc varchar(10))");
+ st.execute("insert into d3253 values " +
+ "(1, 'one'), (2, 'two'), (3, 'three'), (1, 'un')");
+
+ /* Before DERBY-3253 was fixed, this query would have thrown
+ * an execution time NPE due to the fact the generated column
+ * reference was pointing to the wrong place.
+ */
+ JDBC.assertUnorderedResultSet(st.executeQuery(
+ "select x.* from d3253, (select * from d3253) x " +
+ "where d3253.i = x.i and x.vc in ('un', 'trois')"),
+ new String [][] {{"1","un"},{"1","un"}});
+
+ JDBC.assertUnorderedResultSet(st.executeQuery(
+ "select x.* from d3253, (select * from d3253) x " +
+ "where d3253.i = x.i and x.i in (2, 3)"),
+ new String [][] {{"2","two"},{"3","three"}});
+
+ JDBC.assertEmpty(st.executeQuery(
+ "select x.* from d3253, (select * from d3253) x " +
+ "where d3253.i = x.i and x.vc in ('uno', 'tres')"));
+
+ st.execute("drop table d3253");
st.close();
}
Re: Encouraging useful commit messages WAS Re: svn commit: r606277
Posted by Andrew McIntyre <mc...@gmail.com>.
On Dec 21, 2007 11:53 AM, Daniel John Debrunner <dj...@apache.org> wrote:
> abrown@apache.org wrote:
> > Author: abrown
> > Date: Fri Dec 21 11:21:53 2007
> > New Revision: 606277
> >
> > URL: http://svn.apache.org/viewvc?rev=606277&view=rev
> > Log:
> > DERBY-3253: Port changes to 10.3.
>
> Would it be possible to have a more useful commit message that gives
> some indication of the change. It makes svn log much more readable by
> itself, without having to lookup information in the jira system.
>
> This helps when looking for changes that might have broken a test, a
> useful commit message can help in determining if change is likely to
> have broken a specific area or not.
Let me add to this that when merging changes between branches, it is a
good idea to include the specific revisions that you merged, and
whether or not additional changes were necessary. This helps others
when porting to another branch further back and helps to prevents
duplicate merges by keeping a list of merged revisions in the history
that a person can check against.
andrew
Encouraging useful commit messages WAS Re: svn commit: r606277
Posted by Daniel John Debrunner <dj...@apache.org>.
abrown@apache.org wrote:
> Author: abrown
> Date: Fri Dec 21 11:21:53 2007
> New Revision: 606277
>
> URL: http://svn.apache.org/viewvc?rev=606277&view=rev
> Log:
> DERBY-3253: Port changes to 10.3.
Would it be possible to have a more useful commit message that gives
some indication of the change. It makes svn log much more readable by
itself, without having to lookup information in the jira system.
This helps when looking for changes that might have broken a test, a
useful commit message can help in determining if change is likely to
have broken a specific area or not.
Dan.