Обсуждение: Re: [PORTS] Port Bug Report: int2 negative numbers not parsed correctly

> > I declared a column to be type "smallint".  It works, except
> > when I attempt to enter a negative number at which time the
> > parser complains about its 'type'. See example, below.
> > mdalphin=> insert into test values (-1);
> > ERROR:  parser: attribute 'number' is of type 'int2' but expression
> > is of type 'int4'
> This is a problem we have seen in a few places.  It is caused by the
> negative sign being handled in an unusual way.  No fix known yet.

There are two ways to address this for a fix as far as I can tell:

1) in the parser transformations (and/or in the optimizer), look for
unary minus operators on constants, and convert those node subtrees to
negative constant nodes.

2) try to do the right thing to convert types to be compatible with
target columns. I'm working on this topic now, but I'm planning on
addressing functions (first cut is done) and operators (starting now)
before looking at target columns. Hopefully all three areas will be
do-able.

Anyone interested in looking at (1)? I think it would be a good thing to
have even if (2) masks the problem away, unless of course the optimizer
already gets rid of function calls on constants by executing them before
run-time...

                   - Tom

> 1) in the parser transformations (and/or in the optimizer), look for
> unary minus operators on constants, and convert those node subtrees to
> negative constant nodes.
>
> 2) try to do the right thing to convert types to be compatible with
> target columns. I'm working on this topic now, but I'm planning on
> addressing functions (first cut is done) and operators (starting now)
> before looking at target columns. Hopefully all three areas will be
> do-able.
>
> Anyone interested in looking at (1)? I think it would be a good thing to
> have even if (2) masks the problem away, unless of course the optimizer
> already gets rid of function calls on constants by executing them before
> run-time...

I am confused.  As I can tell, these are coming in as null_expr - 1.
Why can't we do a check in gram.y, and if it is a null_expr - 1, replace
to a negative constant of int or float8.  The regular constant type
conversion code will then fix this.

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

> I am confused.  As I can tell, these are coming in as null_expr - 1.

What is "null_expr - 1"? I think that is the same thing; a node with a
subtraction operator and the left side set to null and the right side
set to a constant node. That's what I meant by the unary minus on a
constant.

> Why can't we do a check in gram.y,...

Well we maybe can, but it sure is ugly. This will be spread around a
bunch of places (everywhere there is a unary minus allowed). I already
did the wrong thing and brute-forced something similar into the CREATE
SEQUENCE code in gram.y. Isolating it in transform_expr() or somewhere
like that would be much cleaner.

>
> > I am confused.  As I can tell, these are coming in as null_expr - 1.
>
> What is "null_expr - 1"? I think that is the same thing; a node with a
> subtraction operator and the left side set to null and the right side
> set to a constant node. That's what I meant by the unary minus on a
> constant.
>
> > Why can't we do a check in gram.y,...
>
> Well we maybe can, but it sure is ugly. This will be spread around a
> bunch of places (everywhere there is a unary minus allowed). I already
> did the wrong thing and brute-forced something similar into the CREATE
> SEQUENCE code in gram.y. Isolating it in transform_expr() or somewhere
> like that would be much cleaner.
>

But isn't it is just one line in gram.y.  That is where I was seeing it
happen.

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

> > Well we maybe can, but it sure is ugly. This will be spread around a
> > bunch of places (everywhere there is a unary minus allowed). I
> > already did the wrong thing and brute-forced something similar into
> > the CREATE SEQUENCE code in gram.y. Isolating it in transform_expr()
> > or somewhere like that would be much cleaner.
> But isn't it is just one line in gram.y.  That is where I was seeing
> it happen.

golem$ grep UMINUS gram.y
%right          UMINUS
                        | '-' default_expr %prec UMINUS
                        | '-' constraint_expr %prec UMINUS
                | '-' a_expr %prec UMINUS
                | '-' b_expr %prec UMINUS
                | '-' position_expr %prec UMINUS

So at least 5 different places, perhaps more when you get into it :(

                     - Tom

>
> > > Well we maybe can, but it sure is ugly. This will be spread around a
> > > bunch of places (everywhere there is a unary minus allowed). I
> > > already did the wrong thing and brute-forced something similar into
> > > the CREATE SEQUENCE code in gram.y. Isolating it in transform_expr()
> > > or somewhere like that would be much cleaner.
> > But isn't it is just one line in gram.y.  That is where I was seeing
> > it happen.
>
> golem$ grep UMINUS gram.y
> %right          UMINUS
>                         | '-' default_expr %prec UMINUS
>                         | '-' constraint_expr %prec UMINUS
>                 | '-' a_expr %prec UMINUS
>                 | '-' b_expr %prec UMINUS
>                 | '-' position_expr %prec UMINUS
>
> So at least 5 different places, perhaps more when you get into it :(

OK, let's take a look at it.  The only one I have seen a problem with
is:

    | '-' a_expr %prec UMINUS

But let's look at the others.  Default_expr has it:

    default_expr:  AexprConst
                    {   $$ = makeConstantList((A_Const *) $1); }
                | NULL_P
                    {   $$ = lcons( makeString("NULL"), NIL); }
                | '-' default_expr %prec UMINUS
                    {   $$ = lcons( makeString( "-"), $2); }

But I don't understand why it is there.  Doesn't AexprConst handle such
a case, or do we get shift-reduce conflicts without it?

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

> > So at least 5 different places, perhaps more when you get into it :(
> OK, let's take a look at it.  The only one I have seen a problem with
> is:
>
>         | '-' a_expr %prec UMINUS
>
> But let's look at the others.  Default_expr has it:
>
>         default_expr:  AexprConst
>                         {   $$ = makeConstantList((A_Const *) $1); }
>                     | NULL_P
>                         {   $$ = lcons( makeString("NULL"), NIL); }
>                     | '-' default_expr %prec UMINUS
>                         {   $$ = lcons( makeString( "-"), $2); }
>
> But I don't understand why it is there.  Doesn't AexprConst handle
> such a case, or do we get shift-reduce conflicts without it?

No, _no_ negative numbers get through scan.l without being separated
into a minus sign and a positive number. This is because the scanner
does not have any information about context, and cannot distinguish the
usage of the minus, for example between the two cases "a - b" and "- b".
So, to handle "a - b" correctly, the minus sign must always be
separated, otherwise you get "a (-b)" which the grammar does not
understand.

The one place which will see every node come through is in the parser
transformations or in the optimizer, which is why it seems that those
might be good places to look for this case.

                   - Tom