"Brendan Jurd" <direvus@gmail.com> writes:
> 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.

Well, that's a reasonable argument. And now that I think about it,
a parser-only node type doesn't have nearly the support overhead that
a full-fledged executable node does. So no objection to A_ArrayExpr
if you want to do that.

> 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.

Uh, you missed quite a lot of others ... see CURRENT_DATE and a lot of
other productions.

regards, tom lane

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

On Nov 28, 2007 9:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > 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.
>
> Uh, you missed quite a lot of others ... see CURRENT_DATE and a lot of
> other productions.
>

Thanks again. I missed those because they don't use
makeStringConst(). Looking again, it turns out "many productions" is
more like 15.

That's a bigger number, certainly, but it's still manageable. It
wouldn't be hard to convert them to generate a const-in-a-cast. In
fact with the addition of a makeCastStringConst(), I think the code
saving from A_Const->typename would be cancelled out.

If the only reason for keeping A_Const->typename around is the alleged
code saving (as indicated by the code comments), my offer to do away
with it is still on the table.

Regards,
BJ

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Brendan Jurd escribió:

> If the only reason for keeping A_Const->typename around is the alleged
> code saving (as indicated by the code comments), my offer to do away
> with it is still on the table.

Code cleanup is always welcome.

--
Alvaro Herrera Developer, http://www.PostgreSQL.org/
"The eagle never lost so much time, as
when he submitted to learn of the crow." (William Blake)

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Hi folks,

The patch is coming along nicely now. I do have a couple of questions
about the implementation in transformArrayExpr though.

----
1) How should we determine whether the array is multidimensional if we
know the type in advance?

Currently, transformArrayExpr uses the results of its search for a
common element type to figure out whether the array is
multidimensional. If we know the type in advance, we don't need to do
the common type search (a nice side-effect), so we need some other way
of figuring out how to set ArrayExpr->multidims on the new node.

I could just check the nodeTag of the elements as they are
transformed, but I'm concerned that the existing code might be relying
on select_common_type to catch stupid input, like a mixture of scalar
and array elements. If that's the case it might be unwise to bypass
select_common_type or, at least, I'd need to come up with something
else to provide the same level of sanity assurance in both code paths.

----
2) Should the typecast propagate downwards into nested array elements?

If we have a nested array written as, say, ARRAY[ARRAY[1, 2], ARRAY[3,
4], ARRAY[5, 6]]::float[], should we treat the inner arrays the same
way as the outer array (with the advance knowledge that the array type
should be float[])?

If I'm reading the code correctly, the end result should be much the
same, because the inner arrays will end up being coerced to float[]
anyway. But shortcutting the coercion could save some cycles.

Comments?

Regards,
BJ

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

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

On Fri, Nov 30, 2007 at 06:13:20AM +1100, Brendan Jurd wrote:
> Hi folks,
>
> The patch is coming along nicely now. I do have a couple of questions
> about the implementation in transformArrayExpr though.

Awesome.

> 1) How should we determine whether the array is multidimensional if we
> know the type in advance?

Well, given the array should be regular you should be able to just look
at the first element, if it's a array look at it's first element, etc
to determine the dimensions. This'll be fairly quick.

> 2) Should the typecast propagate downwards into nested array elements?

IMHO yes, you have th einfo you may as well use it.

> If we have a nested array written as, say, ARRAY[ARRAY[1, 2], ARRAY[3,
> 4], ARRAY[5, 6]]::float[], should we treat the inner arrays the same
> way as the outer array (with the advance knowledge that the array type
> should be float[])?

TBH, I think you're going to have to go through the whole array to
coerce them and check, so you may as well determine the dimensions at
the same time. In general I think it's better to mark the type up
front.

In don't know if you should actually do the conversion straight away,
but at least you don't need to guess the type anymore.

Hope this helps,

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
> Those who make peaceful revolution impossible will make violent revolution inevitable.
> -- John F Kennedy

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD4DBQFHTxeAIB7bNG8LQkwRAneZAJd3lgC+Q87CWHuU5MyVFH 0oamsEAJ9hOD3Q
T21B4NmgM9GKppaUDX6JHw==
=fglL
-----END PGP SIGNATURE-----

Martijn van Oosterhout <kleptog@svana.org> writes:
>> 1) How should we determine whether the array is multidimensional if we
>> know the type in advance?

> Well, given the array should be regular you should be able to just look
> at the first element, if it's a array look at it's first element, etc
> to determine the dimensions. This'll be fairly quick.

How does that work with non-constant array constructor members?

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

A quick recap: I submitted a patch for empty ARRAY[] syntax back in
November, and as far as I can see it never made it to the patches
list. Gregory suggested a different way of approaching the problem
(quoted below), but nobody commented further about how it might be
made to work.

I'd like to RFC again on Gregory's idea, and if that doesn't bear any
fruit I'd like to submit the patch as-is for review.

Regards,
BJ

On 01/12/2007, Brendan Jurd <direvus@gmail.com> wrote:
> On Nov 30, 2007 9:09 PM, Gregory Stark <stark@enterprisedb.com> wrote:
> > I'm sorry to suggest anything at this point, but... would it be less invasive
> > if instead of requiring the immediate cast you created a special case in the
> > array code to allow a placeholder object for "empty array of unknown type".
> > The only operation which would be allowed on it would be to cast it to some
> > specific array type.
> >
> > That way things like
> >
> > UPDATE foo SET col = array[];
> > INSERT INTO foo (col) VALUES (array[]);
> >
> > could be allowed if they could be contrived to introduce an assignment cast.
>
> Not sure it would be less invasive, but I do like the outcome of being
> able to create an empty array pending assignment. In addition to your
> examples, it might also make it possible to do things like this in
> plpgsql
>
> DECLARE
> a text[] := array[];
>
> Whereas my patch requires you to write
>
> a text[]: =array[]::text[];
>
> ... which seems pretty stupid.
>
....
> Any suggestions about how you would enforce the "only allow casts to
> array types" restriction on the empty array?
>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your Subscription:
http://mail.postgresql.org/mj/mj_www...=pgsql-hackers

On 21/03/2008, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Brendan Jurd" <direvus@gmail.com> writes:
>
> > As discussed on -hackers, this patch allows the construction of an
> > empty array if an explicit cast to an array type is given (as in,
> > ARRAY[]::int[]).
>
>
> Applied with minor fixes; mostly, ensuring that the cast action would
> propagate down to sub-arrays, as in

Great, thanks Tom.

> I was interested to realize that this fix validates the decision to
> pass down the type information on-the-fly during transformExpr recursion.
> It would have been a lot more painful to do it if we'd taken the A_Const
> approach.
>

Indeed.

> I didn't do anything about removing A_Const's typename field, but I'm
> thinking that would be a good cleanup patch.
>

I'd be happy to take this on. My day job is pretty busy at the moment
but I should be able to submit something in a week or so.

Cheers,
BJ

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers