Mailing-List: contact derby-dev-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Derby Development" <derby-dev@db.apache.org>
Received-SPF: neutral (asf.osuosl.org: local policy)
Message-ID: <4343FA11.90406@debrunners.com>
Date: Wed, 05 Oct 2005 09:06:41 -0700
From: Daniel John Debrunner <djd@debrunners.com>
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
 rv:1.7.3) Gecko/20040910
MIME-Version: 1.0
To: Derby Development <derby-dev@db.apache.org>
Subject: Re: [PATCH]Derby-582 Dynamic parameter should be allowed to be the
 operand of unary operator "-".
References: <d9619e4a0510042120i155b8beal42751bcdbe9a8b2e@mail.gmail.com>
	 <4343DEC0.5070805@debrunners.com>
 <d9619e4a0510050844s47609df6tcbaa6709c61528fc@mail.gmail.com>
In-Reply-To: <d9619e4a0510050844s47609df6tcbaa6709c61528fc@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Mamta Satoor wrote:

> Let me go back to the example sql
> select * from t1 where c1 = -?
> BinaryOperatorNode.bindExpression checks if right operand is a parameter
> node and if yes, it CASTS the right operand to ParameterNode and calls
> setDescriptor method on it.
>   /* Is there a ? parameter on the right? */
>   if (rightOperand.isParameterNode())
>   {
>    /* Set the right operand to the type of the left parameter. */
>    (*(ParameterNode)* rightOperand).setDescriptor(
> leftOperand.getTypeServices());
>   }
> This casting of a parameter operand to ParameterNode and setting the
> descriptor on it is spread all over Derby engine code and rather than
> having to change them all, I decided to make UnaryOperatorNode extend
> ParameterNode so the rest of the type setting code can work without any
> change.
>  
> It's possible that I have been thinking single track, so if anyone have
> any suggestions, do let me know.

Unfortunately I think you have been driven down that track due to the
current code. :-(

One of my credo's is that it doesn't matter how much code needs to be
changed to make a fix, if it makes the code better then it's a good
thing to change all that code.

On this it seems that there is some current confusion in the code,
ValueNode has two methods to set the type, setType and setDescriptor.
So why does ParameterNode need a special set type method setDescriptor,
and the associated required casting?

A possible direction for the fix is:

   - Have an isParameterNode method in UnaryOperatorNode that returns
     its operand's isParameterNode return value.

   - Have a single type setting method setType in ValueNode

   - Have ParameterNode override setType with any logic it needs.

The the code above becomes:

if (rightOperand.isParameterNode())
    rightOperand.setType(leftOperand.getTypeServices())

Maybe I'm missing something in the above that will come out in the
details, but it may be better to address that, rather than the somewhat
unnatural forcing of a UnaryOperatorNode to be a ParameterNode, when
logically it is not a ParameterNode.

I'm willing to help out on any issues you discover on this new path.
Dan.