Regression: The fields of views are not being calculated properly since 10.1.2.4

• Key: DERBY-1633
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: A B
• Reporter: Prasenjit Sarkar
• Component/s: SQL
• Affects Version/s: 10.1.3.1
• Fix Version/s: 10.1.3.2, 10.2.1.6, 10.3.1.4

Description

Database can be assumed to be same as in Derby - 1205 Jira issue

SELECT PORT1.PORT_ID FROM T_RES_PORT PORT1, T_VIEW_ENTITY2PORT ENTITY2PORT WHERE ENTITY2PORT.PORT_ID = PORT1.PORT_ID

This works fine in 10.1.2.1 but fails thereafter complaining that Comparison between INTEGER and CHAR is not supported

for some reason, it thinks one of the PORT_ID columns is a character, when in reality both are integers.

SELECT DISTINCT
ZONE.ZONE_ID ZONE_ID,
PORT2ZONE.ZONE_MEMBER_ID
FROM
T_RES_ZONE ZONE left outer join T_VIEW_PORT2ZONE PORT2ZONE on
ZONE.ZONE_ID = PORT2ZONE.ZONE_ID , T_RES_FABRIC FABRIC

In this query, it is complaining that one of the columns is a VARCHAR and cannot be compared to INTEGER, when clearly this is not the case...

Same issue

Comments

[A B]

Looks like an issue with the predicate scoping that occurs as part of predicate pushdown for UNIONs--see DERBY-805. I will look into it...

[A B]

Attaching a simplified reproduction of the problem that does not rely on the database in DERBY-1205. (But the database in DERBY-1205 allowed me to write this repro, so THANKS to Prasenjit for posting the database initially).

[A B]

Attaching a detailed description of the issue that is causing this regression, and a description of what I believe to be the correct fix. I'm also attaching a first version of the patch that is for review only.

The reason the patch is for review only is that it does not yet have new test cases for catching this regression in the future, and more importantly, because there are some changes in the patch about which I am not very clear. So I'm hoping someone out there who's familiar with predicate "remapping" can provide some input to help clarify some things for me. A description of what I don't yet understand is provided in section V of the document.

I plan to continue looking at this issue to try to finalize my understanding of the changes, but in the meantime, other comments/reviews would be helpful.

The description of the problem, the fix, and my continuing questions can be found in DERBY-1633_v1.html; the first patch (for review only) is d1633_v1_reviewOnly.patch.

[Kathey Marsden]

Marking fix version 10.2

[A B]

Attaching a second version of the patch, d1633_v2.patch, that addresses the regression described in this issue. Also attaching a second version of the HTML file that answers my previous questions based on tracing and exploration of the code (relevant comments are also in the code).

Differences between v1 and v2:

-- "RESOLVE" comments replaced with comments explaining the relevant pieces of code.

-- Fixed a problem in Predicate.isJoinPredicate() so that the method will only return true if the predicate operands reference different tables (ex. T1.j = T1.i is not a join predicate). The need for this was exposed by the other changes for this issue and this small (2-line) change was required to get the predicatePushdown test to run cleanly.

-- Added minor special-case logic to catch situations where predicates don't reference base tables; see code comments for more.

-- Added a set of new tests to lang/predicatePushdown.sql and updated the master file accordingly.

Note that over half of this patch is from the new tests, and a good chunk of what's left is comments, so the patch isn't actually all that big.

I ran derbyall on Red Hat Linux with ibm142 against SANE jars and saw no new failures.

This patch, d1633_v2.patch, is ready for review/commit.

[A B]

Marking Patch Available since the patch, d1633_v2.patch, is ready for review and commit.

[Rick Hillegas]

Thanks for the extensive analysis, Army! This has helped me to understand some of the issues. At this point I'm afraid that I don't have a very firm grasp of the high level problem and I hope that you will teach me more. It is clear that you have studied this tricky piece of code. Please bear with me.

I lost the thread of your explanation when I got to OBS#2 in DERBY-1633_v2.html. I did not understand this statement: "When a predicate is pushed to a UNION that appears in a Select list, the predicate will be "remapped" so that the column references point to their source result columns." I did not grasp how the problem was caused by the UNION. Why does a UNION require this mapping?

Instead, it seemed to me that the remapping was required by what was underneath the UNION. In this case, it is a Permuting Node (that's a term I just made up), that is, an operator which changes column order. In this case, the Permuting Node is a view, although I suppose it could also be a subquery.

It seems to me that positional remapping is required for other predicates that we try to push down, even if no UNION is involved. For instance, I think we need positional remapping in the following cases. And this brings me to the heart of my confusion: Why does predicate pushdown work in the following cases--or does it? If it does work, how is it that UNIONs break the logic? Here are some cases that come to mind:

select *
from
  t1,
  ( select b as x, a as y from t2) v2
where t1.a = v2.y

select *
from ( select b as x, a as y from t2) v2
where v2.y = 1

[A B]

Reply to Rick's comments can be found here:

http://article.gmane.org/gmane.comp.apache.db.derby.devel/26853

Thanks for reviewing, Rick!

[Rick Hillegas]

Thanks for the extra explanation, Army. I think I understand the problem better now.

I have one issue with this patch: In at least one place (Predicate.skipRemap()) your comments call out certain assumptions about the kinds of queries which take this code path. Some day those assumptions may break. When they do, the customer will see a very unhelpful diagnostic (in this case, a casting exception). I would like to see this code react more defensively, either raising a friendlier exception or, preferably, selecting a different albeit less optimal plan. I'm concerned that this regression is itself a symptom of a missed opportunity to validate our assumptions.

I'd also appreciate it if Satheesh could review this patch. He reviewed the original work, DERBY-805, which introduced this regression. I think he is well placed to analyze subtle problems which may be lurking in this tricky code.

Thanks again for your extensive explanations both in the code and in this JIRA.

[A B]

Thank you Rick for the review and for the helpful comments. I looked through the patch and found two assumptions in the code: one explicit (which you also pointed out) in Predicate.skipRemap(), and another implicit one in the "getScopedOperand()" method of BinaryRelationalOperatorNode.

It's the implicit assumption in getScopedOperand() that is the important one--and some initial tracing shows that there may in fact be a problem in that area. So thanks for bring this up.

I am unchecking the "Patch Available" box as I look into the matter more. In the meantime, committers please note that this means d1633_v2.patch is NOT ready for commit after all. I will post more when I have a better idea of what's going on. Thanks for everyone's patience here...

[Mike Matrigali]

for what it is worth d1633_v2.patch passed a full set of nightly tests on XP, sun jdk 1.4.2. I will wait for revised patch to before committing.

[A B]

Quick update on where I stand with this issue...

When I was tracing through code for the _v2 patch for this issue, I found two things that caused me to withdraw the patch from potential commit.

First, there is some special case logic in BinaryRelOpNode.getScopedOperand() for situations when the scope target ResultColumn could not be found:

+ /* Special case: if the cr has already been "scoped" once and
+ * it corresponds to a ResultColumn whose expression isn't a
+ * ColumnReference (see last line in this method) then rc will
+ * be null here. In that case, we just return a clone of cr.
+ */
+ if (rc == null)
+ return (ValueNode)cr.getClone();

This is the "implicit assumption" that I mentioned in my previous comment, and when I was tracing through code I realized that this assumption was not correct. As it turns out, for the queries I was running to get to this code, the reason rc was null was NOT because it "corresponds to a ResultColumn whose expression isn't a Column Reference". Rather, it was because the subquery that the operand-to-be-scoped ("cr") was pointing to had actually been flattened during preprocessing. To make a long story short, this meant that the ResultColumn to which "cr" was pointing was redundant and thus was not really pointing to the correct source result set. This in turn meant that we tried to find "cr"s (redundant) source result in the childRSN's result column list, we couldn't find it, and hence rc was null. What we need to do, then, is skip over the redundant source result set to find the actual result set, and then search for that in childRSN's RCL.

Having figured out the cause for this special-case logic (namely, subquery flattening when preprocessing), I have been able to make the appropriate changes and have thus removed the need for this special-case logic altogether. So that issue has been resolved.

The second reason I withdrew the patch was for cases where "cr" does actually correspond to a ResultColumn whose expression isn't a ColumnReference. Namely, at the very end of the getScopedOperand() method:

        /* We can get here if the ResultColumn's expression isn't a
         * ColumnReference. For example, the expression would be a
         * constant expression if childRSN represented something like:
         *
         * select 1, 1 from t1
         *
         * In this case we just return a clone of the column reference
         * because it's scoped as far as we can take it.
         */
        return (ValueNode)cr.getClone();

As I was tracing through code to figure out the first issue mentioned above, I noticed that this logic was not updated to reflect the rest of the changes for the _v2 patch. In particular, the _v2 algorithm for finding the scope target column involves just retrieving the "whichRC"-th column of childRSN's result column list if childRSN is the right child of a Union. With the above code, if the scope target's expression is not a ColumnReference we'll just return a clone of "cr"--but "cr" will always be w.r.t to the left child, so if childRSN is actually the right child of a Union, then returning a clone of "cr" is not the correct thing to do. Instead, we have to return a new ColumnReference whose source is set to be the whichRC-th result column in childRSN's result column list.

I have figured out how to implement this second change, as well, and thus have addressed both of my concerns with the _v2 patch.

However, it is still not clear to me why all of my tests--and derbyall, too--passed with these errors in the code. So I still need to try to write more tests to either 1) prove that the _v2 code was wrong (and that my latest changes are correct) or 2) understand why having apparently incorrect code in the _v2 patch still makes everything work.

It is this latter task that I'm still investigating. I will post more when I have more to post...

[A B]

Attaching a third attempt at resolving this issue, d1633_v3_code.patch and d1633_v3_tests.patch. I've separated the tests out from the patch because the test diff for lang/predicatePushdown.out is several thousand lines (that's what happens when code changes affect query plans) and I didn't want it to interefere with review of the code changes. I did manually verify all of the new query plans and while the plans have changed in some ways, they still correctly demonstrate the behavior that is being tested.

I'm also attaching a _v3 of the HTML file to described the _v3 code changes. In particular there is a new section "V" that describes several changes which are in _v3 but were not in _v2. Test cases demonstrating the short-comings of _v2 are included in d1633_v3_tests.patch.

I ran derbyall on Red Hat Linux with ibm142 using sane jars against a patch that was very similar to _v3 and saw no new failures. I did, however, make some additional changes when I created _v3 and so need to re-run derbyall again to make sure all is okay.

In the meantime, though, I'm posting the d1633_v3 patches, which I believe to be a complete solution to the regression described for this issue. As always, review/comments/commit would be greatly appreciated.

svn stat:

M java\engine\org\apache\derby\impl\sql\compile\Predicate.java
M java\engine\org\apache\derby\impl\sql\compile\SelectNode.java
M java\engine\org\apache\derby\impl\sql\compile\ProjectRestrictNode.java
M java\engine\org\apache\derby\impl\sql\compile\BinaryRelationalOperatorNode.java
M java\engine\org\apache\derby\impl\sql\compile\SetOperatorNode.java
M java\engine\org\apache\derby\impl\sql\compile\OptimizerImpl.java
M java\engine\org\apache\derby\impl\sql\compile\ColumnReference.java
M java\engine\org\apache\derby\impl\sql\compile\ResultColumnList.java
M java\testing\org\apache\derbyTesting\functionTests\tests\lang\predicatePushdown.sql
M java\testing\org\apache\derbyTesting\functionTests\master\predicatePushdown.out

[A B]

I applied the _v3 patches and ran derbyall on Red Hat Linux with ibm142 against sane jars with no new failures. So as mentioned in my previous comment, the d1633_v3_code.patch and d1633_v3_tests.patch patches are ready for review and commit.

[Rick Hillegas]

Hi Army. Thanks for the new patch and the extensive explanation in this JIRA and in the code. I am inclined to check in this fix tomorrow unless someone objects.

[Rick Hillegas]

Committed the d1633_v3 patches at subversion revision 434111.

[A B]

Thanks so much for the review and commit, Rick! (it looks like I'm not getting Jira emails, so I didn't see this comment until just now).

Are you planning/willing to port this to the 10.2 branch, as well?

[Rick Hillegas]

Hi Army. Yes, I will port this to 10.2 too.

[Rick Hillegas]

Turning off the "patch available" bit since this has been committed to the trunk.

[Rick Hillegas]

Ported DERBY-1633 (434111) to 10.2 at subversion revision 436921.

[A B]

Attaching a patch to port this fix to the 10.1 codeline, which is where the regression was first introduced. A simple merge command was not possible because other changes (esp. DERBY-781) were checked into the 10.2/10.3 codeline that are not in 10.1 and thus the patch wouldn't merge cleanly.

I applied the patch, d1633_10_1_merge.patch, and ran derbyall with 10.1 sane jars on Red Hat Linux using ibm142. There were no unexpected failures.

So the patch is ready for commit into the 10.1 codeline. Once that's done, I believe this issue and be resolved and closed.

[A B]

Checking "Patch Available" because the patch to merge this change to 10.1 is now available: d1633_10_1_merge.patch.

[Mike Matrigali]

committed d1633_10_1_merge.patch to the 10.1 branch.

m101_142:48>svn commit

Sending java\client\org\apache\derby\client\am\SectionManager.java
Sending java\engine\org\apache\derby\impl\sql\compile\BinaryRelationalOperatorNode.java
Sending java\engine\org\apache\derby\impl\sql\compile\ColumnReference.java
Sending java\engine\org\apache\derby\impl\sql\compile\OptimizerImpl.java
Sending java\engine\org\apache\derby\impl\sql\compile\Predicate.java
Sending java\engine\org\apache\derby\impl\sql\compile\ProjectRestrictNode.java
Sending java\engine\org\apache\derby\impl\sql\compile\ResultColumnList.java
Sending java\engine\org\apache\derby\impl\sql\compile\SelectNode.java
Sending java\engine\org\apache\derby\impl\sql\compile\SetOperatorNode.java
Sending java\testing\org\apache\derbyTesting\functionTests\master\predicatePushdown.out
Sending java\testing\org\apache\derbyTesting\functionTests\tests\lang\predicatePushdown.sql
Transmitting file data ...........
Committed revision 437718

[A B]

Many thanks to Rick and to Mike for porting this fix back to 10.2 and 10.1, respectively.

I confirmed that the change is in 10.1 and als in the latest 10.2 beta snapshot. Also, the new test cases in lang/predicatePushdown.sql run correctly in both codelines. Since this fix is now in trunk, 10.2, and 10.1, I'm marking it resolved and clearing the "patch available" flag.

Thanks also to Rick for taking the time to review the different iterations of this rather involved patch. I appreciate your time!

[Prasenjit Sarkar]

I dont think this issue is resolved yet: I got a SQL error (same as above) for the following query that works in 10.1.2.1:

SELECT HOST2FABRIC1.FABRIC_ID, HOST2FABRIC1.PREFIX_ID, HOST2FABRIC1.HOST_ID FROM T_VIEW_HOST2FABRIC HOST2FABRIC1, T_VIEW_HOST2FABRIC HOST2FABRIC2 WHERE HOST2FABRIC1.PREFIX_ID = HOST2FABRIC2.HOST_ID AND HOST2FABRIC1.HOST_ID = HOST2FABRIC2.HOST_ID AND HOST2FABRIC2.FABRIC_ID = ?

[A B]

Thanks for continuing to test this particular fix, Prasenjit.

I assume the query in the above comment is supposed to be run agains the same database that's in DERBY-1205? When I try to run it I see:

prepare qFail as '

SELECT

  HOST2FABRIC1.FABRIC_ID,
  HOST2FABRIC1.PREFIX_ID,
  HOST2FABRIC1.HOST_ID

FROM

  T_VIEW_HOST2FABRIC HOST2FABRIC1,
  T_VIEW_HOST2FABRIC HOST2FABRIC2

WHERE

  HOST2FABRIC1.PREFIX_ID = HOST2FABRIC2.HOST_ID
  AND HOST2FABRIC1.HOST_ID = HOST2FABRIC2.HOST_ID
  AND HOST2FABRIC2.FABRIC_ID = ?

';

ERROR 42X04: Column 'HOST2FABRIC1.PREFIX_ID' is either not in any table in the FROM list or appears within a join specification and is outside the scope of the join specification or appears in a HAVIN G clause and is not in the GROUP BY list. If this is a CREATE or ALTER TABLE statement then 'HOST2FABRIC1.PREFIX_ID' is not a column in the target table.

I also see this error when running against 10.1.2. So I'm wondering if this is actually the query you intended to post? Could you double-check and post the exact query that's causing the failure?

Thanks.

[Prasenjit Sarkar]

I'm sorry - I screwed up on the db version and query - turns out for the db version, there is an invalid compairson between CHAR and INTEGER, so we are fine.

However, here's a query that works in 10.1.2.1 but not in 10.2.1.1

SELECT DISTINCT
ZONE.ZONE_ID ZONE_ID,
PORT.PORT_ID PORT_ID,
ENTITY_TO_PORT.TYPE,
ENTITY_TO_PORT.PREFIX_ID,
ENTITY_TO_PORT.ENTITY_ID,
ENTITY_TO_PORT.DISPLAY_NAME,
ENTITY_TO_PORT.PORT_DISPLAY_NAME,
PORT2ZONE.MEMBER_NAME,
PORT2ZONE.ZONE_MEMBER_ID,
PORT.PORT_NUMBER
FROM
T_RES_ZONE ZONE left outer join T_VIEW_PORT2ZONE PORT2ZONE on
ZONE.ZONE_ID = PORT2ZONE.ZONE_ID left outer join T_RES_PORT PORT on
PORT2ZONE.PORT_ID = PORT.PORT_ID left outer join T_VIEW_ENTITY_TO_PORT ENTITY_TO_PORT on
PORT2ZONE.PORT_ID = ENTITY_TO_PORT.PORT_ID and
PORT2ZONE.ZONE_ID = ENTITY_TO_PORT.ZONE_ID, T_RES_FABRIC FABRIC
WHERE PORT2ZONE.ZONE_ID = ZONE.ZONE_ID and
ZONE.FABRIC_WWN = FABRIC.FABRIC_WWN and
FABRIC.FABRIC_ID = 1

Same db as before.

In 10.2.1.1 it gives the following error (should this be a new issue?)


java.sql.SQLException: DERBY SQL error: SQLCODE: -1, SQLSTATE: XJ001, SQLERRMC: java.lang.NullPointerExceptionXJ001.U
at org.apache.derby.client.am.SQLExceptionFactory.getSQLException(Unknown Source)
at org.apache.derby.client.am.SqlException.getSQLException(Unknown Source)
at org.apache.derby.client.am.Connection.prepareStatement(Unknown Source)
at org.eclipse.aperi.server.guireq.topology.views.ViewerSanL1.init(ViewerSanL1.java:1828)
at org.eclipse.aperi.server.guireq.topology.views.ViewerInit.init(ViewerInit.java:41)
at org.eclipse.aperi.server.guireq.topology.views.ViewerInit.main(ViewerInit.java:69)
Caused by: org.apache.derby.client.am.SqlException: DERBY SQL error: SQLCODE: -1, SQLSTATE: XJ001, SQLERRMC: java.lang.NullPointerExceptionXJ001.U
at org.apache.derby.client.am.Statement.completeSqlca(Unknown Source)
at org.apache.derby.client.net.NetStatementReply.parsePrepareError(Unknown Source)
at org.apache.derby.client.net.NetStatementReply.parsePRPSQLSTTreply(Unknown Source)
at org.apache.derby.client.net.NetStatementReply.readPrepareDescribeOutput(Unknown Source)
at org.apache.derby.client.net.StatementReply.readPrepareDescribeOutput(Unknown Source)
at org.apache.derby.client.net.NetStatement.readPrepareDescribeOutput_(Unknown Source)
at org.apache.derby.client.am.Statement.readPrepareDescribeOutput(Unknown Source)
at org.apache.derby.client.am.PreparedStatement.readPrepareDescribeInputOutput(Unknown Source)
at org.apache.derby.client.am.PreparedStatement.flowPrepareDescribeInputOutput(Unknown Source)
at org.apache.derby.client.am.PreparedStatement.prepare(Unknown Source)
at org.apache.derby.client.am.Connection.prepareStatementX(Unknown Source)
... 4 more

[A B]

Does the above query work if you run it against Derby embedded instead of client/server? If it works in embedded but not with client/server, then I think this should be filed as a separate issue (against client or server; not sure which). If it fails to execute in embedded mode, too, then please post the stack trace here and we can use that to determine if the failure is related to this issue or if it needs its own Jira.

[Prasenjit Sarkar]

Our application is too network-server centric to run in embedded mode. I would like to help you in any other way, but retrofitting our application is not feasible at this point.

On the whole, it looks like a new issue.

[A B]

> Our application is too network-server centric to run in embedded mode.

Actually, you wouldn't need to change your application at all. You have the database and you have the query. So if you have a full set of Derby jars (ex. the 10.2 beta jars), you could just put the query into a file and execute it directly using Derby's "ij" tool.

Ex. Create a file called "mySQLFile.sql" with contents as follows:

<begin contents>

-- The following URL is for the embedded driver
connect 'jdbc:derby:TPCDB';

set schema tpc;
elapsedtime on;

prepare qFail as '

SELECT DISTINCT

  ZONE.ZONE_ID ZONE_ID,
  PORT.PORT_ID PORT_ID,
  ENTITY_TO_PORT.TYPE,
  ENTITY_TO_PORT.PREFIX_ID,
  ENTITY_TO_PORT.ENTITY_ID,
  ENTITY_TO_PORT.DISPLAY_NAME,
  ENTITY_TO_PORT.PORT_DISPLAY_NAME,
  PORT2ZONE.MEMBER_NAME,
  PORT2ZONE.ZONE_MEMBER_ID,
  PORT.PORT_NUMBER

FROM

  T_RES_ZONE ZONE
  left outer join
    T_VIEW_PORT2ZONE PORT2ZONE
  on
    ZONE.ZONE_ID = PORT2ZONE.ZONE_ID
  left outer join
    T_RES_PORT PORT
  on
    PORT2ZONE.PORT_ID = PORT.PORT_ID
  left outer join
    T_VIEW_ENTITY_TO_PORT ENTITY_TO_PORT
  on
    PORT2ZONE.PORT_ID = ENTITY_TO_PORT.PORT_ID and
    PORT2ZONE.ZONE_ID = ENTITY_TO_PORT.ZONE_ID,
  T_RES_FABRIC FABRIC

WHERE

  PORT2ZONE.ZONE_ID = ZONE.ZONE_ID and
  ZONE.FABRIC_WWN = FABRIC.FABRIC_WWN and
  FABRIC.FABRIC_ID = 1

';

execute qFail;

<end contents>

Then do:

> java org.apache.derby.tools.ij
ij> run 'mySQLFile.sql';

That should be all you need to do. But I see you've filed a separate issue already, so that's fine :) Thanks for following through...

[A B]

The problem described in this Jira issue has been resolved. The remaining failures reported by Prasenjit are now being tracked as DERBY-1777, so I'm closing this issue. Please see DERBY-1777 for more on the NPE.