SEO

On Nov 26, 2007 3:58 AM, Martijn van Oosterhout <kleptog@svana.org> wrote:
> On Mon, Nov 26, 2007 at 03:51:37AM +1100, Brendan Jurd wrote:
> > I noticed in the 8.3 release notes that ARRAY(SELECT ...) now returns
> > an empty array if there are no rows returned by the subquery.
>
> This has come up before, Tom had an idea about how to fix it:
>
> http://groups.google.com/group/pgsql...035035aeaac399
> http://www.mail-archive.com/pgsql-ge.../msg90681.html

[moving thread to -hackers]

Thanks for the link Martijn. I'd be interested in taking a swing at
this if nobody else has laid claim. Since that thread died back in
January, I'm guessing it's wide open.

Regards,
BJ

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Quoting Tom, from the previous thread linked by Martijn:

> It could be pretty ugly, because type assignment normally proceeds
> bottom-up :-(. What you might have to do is make the raw grammar
> representation of ARRAY[] work like A_Const does, ie, there's a
> slot to plug in a typecast. That's pretty much vestigial now for
> A_Const, if memory serves, but it'd be needful if ARRAY[] has to
> be able to "see" the typecast that would otherwise be above it in
> the parse tree.

This approach is making sense to me, but I've run into a bit of a
dependency issue. A_Const does indeed have a slot for typecasts by
way of a TypeName member. A_Const and TypeName are both defined in
parsenodes.h, whereas ArrayExpr is defined in primnodes.h. So
unfortunately I can't just add a TypeName member to ArrayExpr.

I'm new to this area of the codebase (and parsers generally), so I'm
treading carefully. What would be the best way to resolve this?
Would moving TypeName into primnodes.h be acceptable?

Thanks for your time,
BJ

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

"Brendan Jurd" <direvus@gmail.com> writes:
> This approach is making sense to me, but I've run into a bit of a
> dependency issue. A_Const does indeed have a slot for typecasts by
> way of a TypeName member. A_Const and TypeName are both defined in
> parsenodes.h, whereas ArrayExpr is defined in primnodes.h. So
> unfortunately I can't just add a TypeName member to ArrayExpr.

That would be quite the wrong thing to do anyway, since ArrayExpr is
a run-time representation and shouldn't have any such thing attached
to it. What you probably need is a separate parse-time representation
of ARRAY[], a la the difference between A_Const and Const.

Another possibility is to just hack up a private communication path
between transformExpr and transformArrayExpr, ie when you see TypeCast
check to see if its argument is ArrayExpr and do something different.
This would be a mite klugy but it'd be a much smaller patch that way.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

"Brendan Jurd" <direvus@gmail.com> writes:
> I'm not 100% clear on what the A_ prefix signifies ... is A_ArrayExpr
> a good name for the parse-time structure?

Yeah, might as well use that for consistency. The A_ doesn't seem
very meaningful to me either, but I don't want to rename the existing
examples ...

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

On Nov 27, 2007 8:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Brendan Jurd" <direvus@gmail.com> writes:
> > ... So
> > unfortunately I can't just add a TypeName member to ArrayExpr.
>
> That would be quite the wrong thing to do anyway, since ArrayExpr is
> a run-time representation and shouldn't have any such thing attached
> to it. What you probably need is a separate parse-time representation
> of ARRAY[], a la the difference between A_Const and Const.
>

Ah. I wasn't aware of the distinction; I started by looking in gram.y
and saw that the ARRAY parse path creates an ArrayExpr node, whilst
the constant parse paths create A_Const nodes. I didn't realise that
ArrayExpr was "skipping ahead" and creating the same kind of object
that the transform produces.

Glad I stopped and asked for directions then. =)

I'm not 100% clear on what the A_ prefix signifies ... is A_ArrayExpr
a good name for the parse-time structure?

Thanks for your time,
BJ

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

So far I've only considered the '::' cast syntax suggested in the
original proposal, e.g.:

ARRAY[]::text[]

I wonder whether we are also interested in catching CAST(), e.g.:

CAST(ARRAY[] AS text[])

I'm personally okay with leaving it at support for '::', but
admittedly I am heavily biased towards this syntax (I find CAST very
ugly). I suppose supporting CAST as well would be the more
predictable behaviour; I think people might be surprised if we
supported one form of casting but not the other.

Comments?

Regards,
BJ

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

"Brendan Jurd" <direvus@gmail.com> writes:
> So far I've only considered the '::' cast syntax suggested in the
> original proposal, e.g.:

> ARRAY[]::text[]

> I wonder whether we are also interested in catching CAST(), e.g.:

> CAST(ARRAY[] AS text[])

I think you'll find that it's just about impossible to not handle both,
because they look the same after the grammar gets done.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

On Nov 28, 2007 2:56 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I wonder whether we are also interested in catching CAST(), e.g.:
>
> > CAST(ARRAY[] AS text[])
>
> I think you'll find that it's just about impossible to not handle both,
> because they look the same after the grammar gets done.

Thanks Tom ... your comment makes me suspect I've been barking up the
wrong tree.

My original intent was to modify the grammar rules to catch an array
expression followed by a typecast, and put the target typename of the
cast directly into the A_ArrayExpr struct. That notion came from
looking at the way that TypeName gets put into A_Const --
makeStringConst() takes an optional TypeName argument.

Looking at the code in the context of your comment, that was probably
a bad approach. I may've taken the A_Const analogy too far.

Now I'm thinking I leave the grammar rules alone (apart from making it
legal to specify an empty list of elements), and instead push the
typename down into the child node from makeTypeCast(), if the child is
an A_ArrayExpr. Does that work better?

Regards,
BJ

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

"Brendan Jurd" <direvus@gmail.com> writes:
> Now I'm thinking I leave the grammar rules alone (apart from making it
> legal to specify an empty list of elements), and instead push the
> typename down into the child node from makeTypeCast(), if the child is
> an A_ArrayExpr. Does that work better?

Actually, if you do that you might as well forego the separate node type
(which requires a nontrivial amount of infrastructure). I think it
would work just about as well to have transformExpr check whether the
argument of a TypeCast is an ArrayExpr, and if so call
transformArrayExpr directly from there, passing the TypeName as an
additional argument. Kinda ugly, but not really any worse than the way
A_Const is handled in that same routine. (In fact, we could use the
same technique to get rid of the typename field in A_Const ... might
be worth doing?)

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

On Nov 28, 2007 4:19 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Brendan Jurd" <direvus@gmail.com> writes:
> > Now I'm thinking I leave the grammar rules alone (apart from making it
> > legal to specify an empty list of elements), and instead push the
> > typename down into the child node from makeTypeCast(), if the child is
> > an A_ArrayExpr. Does that work better?
>
> Actually, if you do that you might as well forego the separate node type
> (which requires a nontrivial amount of infrastructure). I think it
> would work just about as well to have transformExpr check whether the
> argument of a TypeCast is an ArrayExpr, and if so call
> transformArrayExpr directly from there, passing the TypeName as an
> additional argument.

I actually thought that A_ArrayExpr would be a good addition even if
you ignore the matter of typecasting. It always seemed weird to me
that the parser generates an ArrayExpr directly. ArrayExpr has a
bunch of members that are only set by the transform; all the parser
does is set the 'elements' member. And then the transform creates a
brand new ArrayExpr and populates it based on what's in the 'elements'
member of the otherwise-empty ArrayExpr passed to it.

So my feeling is that an A_ArrayExpr is a better fit for the parser
output than ArrayExpr, and more in keeping with how the rest of the
code does things.

Mind you I'm also okay with your suggestion to let transformExpr take
care of it. But I'm not adverse to putting in the legwork to set up
the infrastructure for A_ArrayExpr, if it's a nice outcome.

> Kinda ugly, but not really any worse than the way
> A_Const is handled in that same routine. (In fact, we could use the
> same technique to get rid of the typename field in A_Const ... might
> be worth doing?)

I had a bit of a dig into this. A_Const->typename gets set directly
by the parse paths for "INTERVAL [(int)] string [interval range]". In
fact, as far as I can tell that's the _only_ place A_Const->typename
gets used at all. And all the transform does with that piece of
information is treat the node like a typecast.

I'm not seeing a huge amount of value in this special treatment. Why
not just have the parser build this as an A_Const inside a TypeCast
and then let the transform deal with it in the usual way? I found the
following comment at parsenodes.h:244

* NOTE: for mostly historical reasons, A_Const parsenodes contain
* room for a TypeName; we only generate a separate TypeCast node if the
* argument to be casted is not a constant. In theory either representation
* would work, but the combined representation saves a bit of code in many
* productions in gram.y.

However, this is no longer the case. makeTypeCast() doesn't care
about whether its argument is a constant anymore:

* Earlier we would determine whether an A_Const would
* be acceptable, however Domains require coerce_type()
* to process them -- applying constraints as required.

And in "many productions in gram.y", "many" == 2. Currently the
combined representation requires more code than it saves.

So, I get the impression the use-case for A_Const->typename has become
extinct. I think it could be removed with a minimum of fuss, and I'd
be happy to include same with my patch (or, submit it as a separate
patch; let me know your preference).

Regards,
BJ

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org