Mailing-List: contact derby-dev-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: <derby-dev@db.apache.org>
Received-SPF: neutral (asf.osuosl.org: local policy)
Message-ID: <43A1EFA1.1080203@Sourcery.Org>
Date: Thu, 15 Dec 2005 14:35:13 -0800
From: Satheesh Bandaram <satheesh@Sourcery.Org>
User-Agent: Mozilla Thunderbird 0.7.3 (Windows/20040803)
MIME-Version: 1.0
To: derby-dev@db.apache.org
Subject: Re: [jira] Updated: (DERBY-649) Useful indexes not used in UNION
 ALL
References: <1060524664.1134627585892.JavaMail.jira@ajax.apache.org>
 <43A1AA79.60005@sun.com> <43A1BA2D.5050004@Sourcery.Org>
 <43A1D57F.6090106@debrunners.com>
In-Reply-To: <43A1D57F.6090106@debrunners.com>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Some answers below... Unless you spend sometime with complex areas like
optimizer, it is pretty difficult to understand what one function or a
specific change is trying to do in the larger scheme of things. I do see
some points here about comments that I will improve in the code.

Daniel John Debrunner wrote:

>I would avoid usin 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. :-)
>  
>
PushExpression() tries to push an expression and could decide it can't,
for many reasons. If a predicate qualifies some of the conditions, it
would get pushed. (or copied for union case)

>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.
>  
>
OK. I will cleanup comments that don't belong here. I guess I have mixed
comments about what a function does and how it is used by the caller. I
can separe the two.

>   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.
>
>  
>
Again, the same problem as above.. mixing the use with what this
function does. I think in this case, it may be useful to list what kind
of predicates a unionNode can currently push and what it can't, because
this is the main entry point that is specific to unions. Each type of
node can only handle some specific types of predicates and it is good to
list them at the entry point, though the decisions are made later.

>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?
>
>  
>
By not calling pushExpressionsIntoSelect(), the code doesn't push
expressions... Where ever I find I currently can't handle the situation,
calling 'continue' in the loop skips pushing that predicate.

>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.
>  
>
We can call it remap if that makes more sense.. I would interpret remap
to mean "change this predicate to point else where". That is not the
case. We are creating a new predicate that matches current one with a
new columnReference. So, is this copy or remap or something else?

>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.
>
>  
>
I don't mind changing method names or variables... if you have any
specific suggestion, I will consider... If I knew a better name, I would
have used it already .. :-)

Satheesh

>Thanks,
>Dan.
>
>
>
>
>
>
>
>
>
>
>
>  
>