A small bug in gram.y

Started by Gokulakannan Somasundaramabout 16 years ago14 messages
#1Gokulakannan Somasundaram
gokul007@gmail.com

Hi,
In the gram.y, under a_expr rule
under the subrule "a_expr NOT SIMILAR TO a_expr %prec SIMILAR"
the action is as follows
{
FuncCall *n = makeNode(FuncCall);
n->funcname = SystemFuncName("similar_escape");
n->args = list_make2($5, makeNullAConst(-1));
n->agg_star = FALSE;
n->agg_distinct = FALSE;
n->func_variadic = FALSE;
n->over = NULL;
n->location = @5;
$$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "!~", $1, (Node
*) n, @2);
}

I think the n->location should be @3.

Thanks,
Gokul.

#2Gokulakannan Somasundaram
gokul007@gmail.com
In reply to: Gokulakannan Somasundaram (#1)
Re: A small bug in gram.y

Hmmm.... no-one else feels this as a bug????

The logic is that a function call is made for "similar" and the position
where SIMILAR occurs is at the third position, but it has been coded that it
is at fifth position.

Thanks,
Gokul.

On Tue, Oct 27, 2009 at 6:51 AM, Gokulakannan Somasundaram <
gokul007@gmail.com> wrote:

Show quoted text

Hi,
In the gram.y, under a_expr rule
under the subrule "a_expr NOT SIMILAR TO a_expr %prec
SIMILAR"
the action is as follows
{
FuncCall *n = makeNode(FuncCall);
n->funcname = SystemFuncName("similar_escape");
n->args = list_make2($5, makeNullAConst(-1));
n->agg_star = FALSE;
n->agg_distinct = FALSE;
n->func_variadic = FALSE;
n->over = NULL;
n->location = @5;
$$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "!~", $1,
(Node *) n, @2);
}

I think the n->location should be @3.

Thanks,
Gokul.

#3Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Gokulakannan Somasundaram (#2)
Re: A small bug in gram.y

Gokulakannan Somasundaram wrote:

Hmmm.... no-one else feels this as a bug????

The logic is that a function call is made for "similar" and the position
where SIMILAR occurs is at the third position, but it has been coded that it
is at fifth position.

The function call is constructed for the similar_escape function, to
construct a regular expression equivalent to the right operand of the
SIMILAR TO. So setting the error location to the right operand seems OK
to me.

However, I note that for the "a_expr SIMILAR TO a_expr" rule we're doing
what you expected and the error location points to SIMILAR. I think we
should change that to behave like NOT SIMILAR TO.

Here's an example that exercises those paths:

postgres=# SELECT 'aa' NOT SIMILAR TO 123;
ERROR: function pg_catalog.similar_escape(integer, unknown) does not exist
LINE 1: SELECT 'aa' NOT SIMILAR TO 123;
^
HINT: No function matches the given name and argument types. You might
need to add explicit type casts.
postgres=# SELECT 'aa' SIMILAR TO 123;
ERROR: function pg_catalog.similar_escape(integer, unknown) does not exist
LINE 1: SELECT 'aa' SIMILAR TO 123;
^
HINT: No function matches the given name and argument types. You might
need to add explicit type casts.
postgres=#

I think the former error location is better.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Heikki Linnakangas (#3)
Re: A small bug in gram.y

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Here's an example that exercises those paths:

postgres=# SELECT 'aa' NOT SIMILAR TO 123;
ERROR: function pg_catalog.similar_escape(integer, unknown) does not exist
LINE 1: SELECT 'aa' NOT SIMILAR TO 123;
^
HINT: No function matches the given name and argument types. You might
need to add explicit type casts.
postgres=# SELECT 'aa' SIMILAR TO 123;
ERROR: function pg_catalog.similar_escape(integer, unknown) does not exist
LINE 1: SELECT 'aa' SIMILAR TO 123;
^
HINT: No function matches the given name and argument types. You might
need to add explicit type casts.
postgres=#

I think the former error location is better.

FWIW, I like the second one better, and if you check around you'll find
that it matches most other similar stuff, eg

regression=# select 12 like 34;
ERROR: operator does not exist: integer ~~ integer
LINE 1: select 12 like 34;
^
HINT: No operator matches the given name and argument type(s). You might need to add explicit type casts.

I think the current coding probably is just a typo, but hadn't gotten
around to doing anything about it.

regards, tom lane

#5Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Heikki Linnakangas (#3)
Re: A small bug in gram.y

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:

LINE 1: SELECT 'aa' NOT SIMILAR TO 123;
^

LINE 1: SELECT 'aa' SIMILAR TO 123;
^

I think the former error location is better.

So do I.

-Kevin

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#5)
Re: A small bug in gram.y

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:

LINE 1: SELECT 'aa' NOT SIMILAR TO 123;
^

LINE 1: SELECT 'aa' SIMILAR TO 123;
^

I think the former error location is better.

So do I.

Uh, why? It looks like it's complaining about the constant 123,
not about the operator.

regards, tom lane

#7Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#6)
Re: A small bug in gram.y

Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:

LINE 1: SELECT 'aa' NOT SIMILAR TO 123;
^

LINE 1: SELECT 'aa' SIMILAR TO 123;
^

I think the former error location is better.

So do I.

Uh, why? It looks like it's complaining about the constant 123,
not about the operator.

I wrote that before I saw your post, which left me ambivalent. My
thinking was that it seems clearest to me when it points to the token
at which things become untenable. At this point there it is still
possible for the statement to be completed with a valid query:

SELECT 'aa' SIMILAR TO

At this point it is not:

SELECT 'aa' SIMILAR TO 123

If you had something like '123' instead of 123, it would be OK, so I'd
be good with it complaining about the constant 123.

SELECT 'aa' SIMILAR TO '123';
?column?
----------
f
(1 row)

But if we normally point to the operator when it isn't in the set
allowed with the given operands, then consistency trumps the above.
I yield the point.

-Kevin

#8Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Tom Lane (#6)
Re: A small bug in gram.y

Tom Lane wrote:

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:

LINE 1: SELECT 'aa' NOT SIMILAR TO 123;
^

LINE 1: SELECT 'aa' SIMILAR TO 123;
^

I think the former error location is better.

So do I.

Uh, why? It looks like it's complaining about the constant 123,
not about the operator.

The problem *is* in the constant 123. It's of wrong type for SIMILAR TO
operator. I guess your viewpoint is that the operator isn't correct for
the operands. Fair enough.

BTW, the corresponding error in the "SIMILAR TO ... ESCAPE ..." syntax is:

postgres=# SELECT 'aa' SIMILAR TO 123 ESCAPE 'f';
ERROR: function pg_catalog.similar_escape(integer, unknown) does not exist
LINE 1: SELECT 'aa' SIMILAR TO 123 ESCAPE 'f';
^
HINT: No function matches the given name and argument types. You might
need to add explicit type casts.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

#9Tom Lane
tgl@sss.pgh.pa.us
In reply to: Heikki Linnakangas (#8)
Re: A small bug in gram.y

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

BTW, the corresponding error in the "SIMILAR TO ... ESCAPE ..." syntax is:

postgres=# SELECT 'aa' SIMILAR TO 123 ESCAPE 'f';
ERROR: function pg_catalog.similar_escape(integer, unknown) does not exist
LINE 1: SELECT 'aa' SIMILAR TO 123 ESCAPE 'f';
^

Well, that's complaining specifically about the ESCAPE part of it.
This does expose the implementation detail that we try to build the
similar_escape() call before the overall similar() function call.

regards, tom lane

#10Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Tom Lane (#9)
Re: A small bug in gram.y

Tom Lane wrote:

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

BTW, the corresponding error in the "SIMILAR TO ... ESCAPE ..." syntax is:

postgres=# SELECT 'aa' SIMILAR TO 123 ESCAPE 'f';
ERROR: function pg_catalog.similar_escape(integer, unknown) does not exist
LINE 1: SELECT 'aa' SIMILAR TO 123 ESCAPE 'f';
^

Well, that's complaining specifically about the ESCAPE part of it.
This does expose the implementation detail that we try to build the
similar_escape() call before the overall similar() function call.

Yeah. The "ESCAPE 'f'" part is OK. It's still the 123 that's of the
wrong type. I'd say that message should point to 123 as well. Or to
SIMILAR, if we take your stance that the error is with the SIMILAR TO
operator in general. But pointing to ESCAPE is just weird.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kevin Grittner (#7)
Re: A small bug in gram.y

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

Uh, why? It looks like it's complaining about the constant 123,
not about the operator.

I wrote that before I saw your post, which left me ambivalent. My
thinking was that it seems clearest to me when it points to the token
at which things become untenable.

Our error pointers are *not* about how far to the right did parsing
get, they're about which part of the construct seems to be most
directly related to the problem. Otherwise most of them would point
at the ending semicolon ;-). A possibly less flippant example is

select nosuchfunction(1,2,3,avalidfunction(4));
^

select nosuchfunction(1,2,3,avalidfunction(4));
^

Which of these is less likely to be misread about which function is
being complained of?

regards, tom lane

#12Tom Lane
tgl@sss.pgh.pa.us
In reply to: Heikki Linnakangas (#10)
Re: A small bug in gram.y

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

... But pointing to ESCAPE is just weird.

Maybe. The implementation's-eye view of this is that the construct is

data SIMILAR-operator (pattern ESCAPE-operator escape-char)

but I agree that isn't the way a user will think of it. I could see
making the location be that of the SIMILAR keyword for both of the
operators.

regards, tom lane

#13Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Tom Lane (#11)
Re: A small bug in gram.y

Tom Lane <tgl@sss.pgh.pa.us> wrote:

select nosuchfunction(1,2,3,avalidfunction(4));
^

select nosuchfunction(1,2,3,avalidfunction(4));
^

Which of these is less likely to be misread about which function is
being complained of?

Actually, I much prefer what PostgreSQL does. :-)

ERROR: function nosuchfunction(integer, integer, integer, integer)
does not exist
LINE 1: select nosuchfunction(1,2,3,avalidfunction(4));
^
HINT: No function matches the given name and argument types. You
might need to add explicit type casts.

But, anyway, point taken.

-Kevin

#14Tom Lane
tgl@sss.pgh.pa.us
In reply to: Heikki Linnakangas (#10)
Re: A small bug in gram.y

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

... But pointing to ESCAPE is just weird.

I've changed these all to @2 (LIKE, ILIKE, SIMILAR TO).

regards, tom lane