Map List<T> operations to PostgreSQL arrays

[austindrenski]

This is a subset of the work from #431 refactored without IQueryOptimizer or NpgsqlExistsToAnyRewritingExpressionVisitor per #460.

Compatibility

Operator	PostgreSQL support
=	7.4+
<>	7.4+
<	7.4+
>	7.4+
<=	7.4+
>=	7.4+
@>	8.2+
<@	8.2+
&&	8.2+
||	7.4+

Function	PostgreSQL support
array_append(anyarray, anyelement)	see ||
array_cat(anyarray, anyarray)	see ||
array_dims(anyarray)	7.4+
array_fill(anyelement, int[] [, int[]])	8.4+
array_length(anyarray, int)	8.4+
array_lower(anyarray, int)	7.4+
array_ndims(anyarray)	8.4+
array_position(anyarray, anyelement [, int])	9.5+
array_positions(anyarray, anyelement)	9.5+
array_prepend(anyelement, anyarray)	see ||
array_remove(anyarray, anyelement)	9.3+
array_replace(anyarray, anyelement, anyelement)	9.3+
array_to_string (anyarray, text)	7.4-9.0
array_to_string(anyarray, text [, text])	9.1+
array_upper(anyarray, int)	7.4+
string_to_array (text, text)	7.4-9.0
string_to_array(text, text [, text])	9.1+

Postponed

Function	PostgreSQL support
array_agg(expression)	8.4+
cardinality(anyarray)	9.4+
unnest(anyarray)	8.4+
unnest(anyarray, anyarray [, ...])	9.4+

Related

#395
#431
#460
#542
#543
#544
#545
#549
#551
#552

[roji]

I finally got back to this and give it another round. We need to hurry if we really want to get this into 2.2.

Just a reminder to always keep in mind to avoid adding stuff for completeness's sake, and wait until (several) users request things from us - I think some features introduced in this PR aren't very likely to be used etc.

[roji]

Is Array.Length really given as a MemberExpression in the expression tree? Unless I'm mistaken it has its own NodeType. This is why I had to have special support for it in NpgsqlSqlTranslatingExpressionVisitor.

[austindrenski]

I find it a bit surprising too, but from everything I'm seeing, it is indeed a MemberExpression.

When debugging ArrayQueryTest. I can see that the array property is actually wrapped up as a ColumnExpression, so perhaps that has something to do with it:

[roji]

The point of the previous code was to immediately delegate to base.VisitBinary() in all cases, unless when the expression's node type is ArrayIndex. In the new code you call Visit() on Left and Right, and then it gets called again if the expression isn't ArrayIndex or Index. This could be bad for performance and may potentially have other side-effects - when we're changing the query pipeline I'd rather be as little invasive as we can.

So I'd start with a switch on the NodeType, which immediately returns base.VisitBinary() for anything but what we handle here.

[austindrenski]

I've reworked this into a switch to cut down on the possibility of wasted visits. How does it look now?

[roji]

I'm confused... NpgsqlArrayFragmentTranslator now constructs and returns ArrayAnyAllExpressions, are they expected to find their way back here in the SqlTranslatingExpressionVisitor? Is this because we assume the fragment translators execute before this method? Either way, the whole point of having the FragmentTranslator is to cleanly encapsulate all the array logic in one place, but this seems to spread it back around...

The situation seems to be similar with the other custom expression we have, but I may be misunderstanding completely.

[austindrenski]

I based this on the implementation in SqlTranslatingExpressionVisitor which does similar work on NodeType.Extension expressions defined upstream.

The fragment translators always run first, so the idea is to package up a bunch of complicated nodes into one custom expression that we can easily identify and handle later on in the normal translator workflow.

It does spread around some logic, but this is still advantageous for the NpgsqlSqlTranslatingExpressionVisitor, because we're just performing normal expression visitor operations on ArrayAnyAllExpression (e.g. visit members, return original or modified).

Note we do have one remaining piece of complicated ArrayAnyAllExpression logic outside the fragment translator, which involves translating a ContainsResultOperator. I want to refactor that into a fragment translator too, but I'd like to deal with that in a later PR.

[austindrenski]

@roji Thanks for the review. I'm (slowly) working my way through it, and hope to have it addressed within the next day or two.

[austindrenski]

@roji I've finished going through that latest review. Some notable improvements based on your comments:

• Removed the incomplete support for multidimensional arrays.
• Removed VersionAtLeast checks for unsupported versions of PostgreSQL.
• Moved pseudo-array string translations into NpgsqlStringTranslator.

I would like to get this incorporated during the preview cycles for 3.0, but I don't think there's any real need to merge it before then, so no need to rush on the re-review.

[roji]

@austindrenski I just re-noticed this... It's really unfortunately we didn't get this merged before the new query pipeline work. Do you have time to rebase this to the latest version and do the porting?

[darbio]

Any movement on getting this into the library? It would be good to not have to worry about arrays T[] and just use List in the code base I'm working on.

[roji]

Closing as there has been no comment from @austindrenski for a while, and this PR would probably need to be redone anyway after the query pipeline redo of 3.0. The tracking issue here is #395.