db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Army <qoz...@gmail.com>
Subject Re: [jira] Updated: (DERBY-578) Grouped select from temporary table raises null pointer exception in byte code generator
Date Thu, 15 Jun 2006 23:26:25 GMT
Manish Khettry (JIRA) wrote:
>      [ http://issues.apache.org/jira/browse/DERBY-578?page=all ]
> 
> Manish Khettry updated DERBY-578:
> ---------------------------------
> 
>     Attachment: predicatePushdown.out.patch
> 
> Looks like the join order changed between when I generated the patch and now. 
> Try this version of predicatePushdown.

I haven't had time to follow this issue closely, but the diff you just posted 
caught my attention--somehow these master updates seem "funny" to me (for lack 
of a better technical term).

I glanced through all of the master updates that you've made for this issue and 
they all show two kinds of diffs: the first is that Distinct Scans are no longer 
reporting index usage, and the second is that start/stop position for heap scans 
has gone from "None" to "null".

For example:

-			Distinct Scan ResultSet for T1 using index xxxxFILTERED-UUIDxxxx at read 
committed isolation level using instantaneous share row locking:
+			Distinct Scan ResultSet for T1 at read committed isolation level using 
instantaneous share row locking:

Did your changes actually make it so that the query no longer uses an index on 
T1, or did it just change the logQueryPlan output so that the index is no longer 
being printed?  My guess (having not looked closely at the code) is that your 
changes have just affected logQueryPlan output--i.e. that the query plan itself 
is still what it was before.

If that's true, then I'm not sure I like the idea of losing this index 
information--truth is, index information is one of *the* most useful things (at 
least for me) when I'm reading a logged query plan, so to lose this info seems 
like a less-than-ideal change.

As for the "None" vs "null", I personally prefer the former over the latter--so 
I'm wondering if it's possible to somehow fix the issue described but still 
retain the more meaningful "None" keyword in this output?  This is perhaps being 
a bit picky, but I thought I'd bring it up.

As I said, I haven't had time to do a thorough review, but I did notice the 
following changes in your patch:
	
+        /* helper method used by generateMaxSpecialResultSet and
+         * generateDistinctScan to return the name of the index if the
+         * conglomerate is an index.
+         */
+        private void pushIndexName(ConglomerateDescriptor cd, MethodBuilder mb)
+          throws StandardException
+        {
+            if (cd.isConstraint()) {
+                DataDictionary dd = getDataDictionary();
+                ConstraintDescriptor constraintDesc =
+                    dd.getConstraintDescriptor(tableDescriptor, cd.getUUID());
+                mb.push(constraintDesc.getConstraintName());
+            } else if (cd.isIndex())  {
+                mb.push(cd.getConglomerateName());
+            } else {
+              mb.pushNull("java.lang.String");
+            }
+        }
+	

@@ -3128,7 +3148,7 @@
  		 
mb.push(org.apache.derby.iapi.util.PropertyUtil.sortProperties(tableProperties));
  		else
  			mb.pushNull("java.lang.String");
-		mb.push(cd.getConglomerateName());
+                pushIndexName(cd, mb);
  		mb.push(colRefItem);
  		mb.push(getTrulyTheBestAccessPath().getLockMode());
  		mb.push(tableLockGranularity);

If I'm reading this correctly, we used to _always_ push 
cd.getConglomerateName()--which, if we're using an index, is the name of the 
index--but now, in order to avoid the NPE reported as part of this issue, we 
will only push the conglomerate name if it (the conglomerate) is not a 
constraint.  Logically, I'm not entirely sure that's the correct thing to do, 
since I *think* that a conglomerate can be an index and a constraint at the same 
time (I'd have to investigate more--if anyone out there knows for sure, please 
chime in!).  If that's true, then in situations where we used to have a 
conglomerate that was both an index and a constraint, we would push the index 
name; but now for that same conglomerate we will push the constraint name 
instead, and I'm guessing that the constraint name and the index name are not 
always the same--which might account for the fact the index name is now missing 
from the logQueryPlan?

That's just speculation, though--if anyone can confirm/deny, that'd be great. 
In any event, in order to preserve the existing behavior in logQueryPlan, the 
change might be as small as something like:

+        /* helper method used by generateMaxSpecialResultSet and
+         * generateDistinctScan to return the name of the index if the
+         * conglomerate is an index.
+         */
+        private void pushIndexName(ConglomerateDescriptor cd, MethodBuilder mb)
+          throws StandardException
+        {
+            if (cd.getConglomerateName() != null)
+                mb.push(cd.getConglomerateName());
+            else if (cd.isConstraint()) {
+                DataDictionary dd = getDataDictionary();
+                ConstraintDescriptor constraintDesc =
+                    dd.getConstraintDescriptor(tableDescriptor, cd.getUUID());
+                mb.push(constraintDesc.getConstraintName());
+            } else {
+              mb.pushNull("java.lang.String");
+            }
+        }

Assuming, of course, that constraintDesc.getConstraintName() can't ever be 
null... :)

I didn't test the above suggestion so it may need some tweaking--but if you 
could look at the changes again and/or explain a bit more about what is causing 
the diff in the master files--and hopefully post your findings--then I'd feel a 
lot better about this patch...

For what it's worth,
Army


Mime
View raw message