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
