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.

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:

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.

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

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.

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.