db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel John Debrunner <...@debrunners.com>
Subject Re: [jira] Updated: (DERBY-649) Useful indexes not used in UNION ALL
Date Thu, 15 Dec 2005 20:43:43 GMT
Satheesh Bandaram wrote:

> Ouch... Rick... I have submitted my fix, based on Dan's comments. But I
> will address any input you may have. I am still working on enhancing the
> patch to cover more cases.

Just a few more comments on the comments in this patch. This could be
seen as more in the way of general advice. Good, clear, relevant
comments are essential for more people joining Derby development. The
following is really a dump of my thoughts as I read the comments in the
code for this patch.

I would avoid using subjective words like 'simple', what does that
really mean?

It's may be better to explicitly state what type of predicates are being
pushed down, rather than what isn't being pushed down. Always clearer to
state what is being done, a positive, that what is not being done, a
negative. Not sure if I read the comments as they stand, that it's
totally clear what can be pushed down. I think the code is hampered
somewhat by existing names, e.g. methods called pushExpressesion, and
then you say that expressions cannot be pushed down, but something is
being pushed down in that method! Did the original author really mean
pushPredicate or am I just confused. :-)

And even though I'm asking for more comments, in some cases one can have
too many comments. In this patch, in several places, you talk about the
type of predicates that cannot be pushed down, but it seems to me it has
no relevance to the method being commented. This confuses me, as I look
for logic matching the comments in the body of the method. Examples are:

   Method: findColumnReferenceInResult
         "Handle the case of single table selects for now. Also if there
is an expression under the result column, it is not possible yet to push
the predicates for now."
          I don't see any matching logic in the method.

   Method: UnionNode.pushExpressions
           All the comment is about what expressions can't be pushed,
but it's not decided in this method, but elsewhere.

These "stray" comments have the problem of being very confusing some
time from now, especially if the restriction is lifted but no-one
updates the method with the stray comment as it is not required to be
modified for the fix. It' also makes other coders very nervous to change
such code, as they can't see where the restriction is being enforced,
therefore they think it's some magic they don't understand.

And finally one real dumb question, in
PredicateList.pushExpressionsIntoSelect I see where you ensure the
predicate is a binary operator and has a column reference, but I can't
see where the 'don't push expressions' logic is?

Also in the same method, looking at the parameter copyPredicate I was
expecting to see code that copied a predicate, but really as the comment
below says, it's re-mapping the predicate.

One of my current itches is to improve general knowledge of the
optimizer, and it seems with the good work and investigation you did for
this fix, it's a great chance to ensure the resulting code with comments
is clear while it is still fresh in your mind.

Another one of my beliefs is that correctly named methods, classes,
fields, variables etc. greatly help in understanding the code.

Don't take this as critism, but instead just showing that code &
comments you think is somewhat obvious because you are an expert in the
area, may not appear that way to others.


View raw message