Cube support (revamped)

[kirkbrauer]

This PR extends the original PR #2808 by @Jimmacle and is updated for the latest version of EFCore.PG.

Continues implementation of npgsql/npgsql#3867.

PostgreSQL Operator/Function	NpgsqlCube API
cube && cube	bool Overlaps(this NpgsqlCube a, NpgsqlCube b)
cube @> cube	bool Contains(this NpgsqlCube a, NpgsqlCube b)
cube <@ cube	bool ContainedBy(this NpgsqlCube a, NpgsqlCube b)
cube -> integer	double NthCoordinate(this NpgsqlCube cube, int n)
cube ~> integer	double NthCoordinateKnn(this NpgsqlCube cube, int n)
cube <-> cube	double Distance(this NpgsqlCube a, NpgsqlCube b)
cube <#> cube	double DistanceTaxicab(this NpgsqlCube a, NpgsqlCube b)
cube <=> cube	double DistanceChebyshev(this NpgsqlCube a, NpgsqlCube b)
cube_union(cube, cube)	NpgsqlCube Union(this NpgsqlCube a, NpgsqlCube b)
cube_inter(cube, cube)	NpgsqlCube Intersect(this NpgsqlCube a, NpgsqlCube b)
cube_enlarge(c cube, r double, n integer)	NpgsqlCube Enlarge(this NpgsqlCube cube, double r, int n)
cube_dim(cube)	int Dimensions
cube_ll_coord(cube, integer)	double LlCoord(int n)
cube_ur_coord(cube, integer)	double UrCoord(int n)
cube_is_point(cube)	bool IsPoint
cube_subset(cube, integer[])	NpgsqlCube ToSubset(params int[] indexes)

A few design notes here that need to be discussed:

1. I have renamed NthCoordinate2 to NthCoordinateKnn as this describes the fact this operator uses K-nearest neighbor (KNN).
2. Since we can't determine which IReadOnlyList<double> is being indexed for the LowerLeft and UpperRight lists, I have retained the LlCoord and UrCoord extension methods. Trying to index the lists directly throws an InvalidOperationException exception.
   • Perhaps it is better to remove these direct array accessors and replace them with the LlCoord and UrCoord methods as mentioned by @Jimmacle instead?
3. All indexes are zero-indexed like C# instead of one-indexed like PostgreSQL, however, this can create some translation difficulties due to the need to call the ConvertToPostgresIndex method every time to increment by one either as a literal or by creating an addition expression.
   • We could just use one-indexed arguments for cube expressions, but it might confuse users, however, implicitly converting to zero-indexed can also be confusing when reading the PostgreSQL docs.

I also noticed that the ToString() method of the NpgsqlCube type does not produce full G17 floating-point representations, we should probably make a follow-up PR to that repository to fix this so I can remove the custom serialization code in this repo.

[roji]

@kirkbrauer this is great, thanks - I wish I had more contributions were this high-quality.

See below for questions/suggestions, but I think this is quite close to being mergeable.

[roji]

As an alternative to these, I think you should be able to simply set the returned type mapping (double) directly in the method translator, no? In fact, that's usually better, since we know at the point of translation exactly what type the method returns (it's documented) - the return type isn't sensitive to context and shouldn't get inferred.

[roji]

Just wanting to make sure that this code fragment is actually needed, as you're now setting the returned type mapping in the method translator (basically my comment above). It might be possible to remove this altogether.

[kirkbrauer]

My understanding is that MakePostgresBinary doesn't have a case for these operators so the return type would be NpgsqlCube instead of double. Here, ApplyTypeMappingOnPostgresBinary is basically patching this problem.

As a simplification, we could just add cases to MakePostgresBinary instead to handle this scenario.

[roji]

Ah, I think I see where I wasn't clear... In the translator, you can simply construct a PgBinaryExpression directly, rather than going through the SqlExpressionFactory. Since the translator basically knows everything about the expression it's going to produce (i.e. it's exact return type), I don't think the expression factory is required here (and it would allow you to remove all this cube-specific custom stuff).

The factory is useful mainly when the same expression kind might be constructed from various places and with different operand types; this allows us to factor the knowledge on what type is returned given what operands (and operator) in a single place (the factory). But since the only place that would ever construct cube-related operators is this translator, we may as well just keep all the logic there.

[roji]

Just a note that this is still pending.

[kirkbrauer]

Ok, I've removed the cube-specific logic here and migrated it back to the cube type mapper. I think this should follow your suggestion above.

[roji]

Took another look, and this all looks - the only thing remaining is the custom expression for the indices which should ideally not be needed. Am happy to merge once that's done (and also to help with that task).

[roji]

Just wanting to make sure that this code fragment is actually needed, as you're now setting the returned type mapping in the method translator (basically my comment above). It might be possible to remove this altogether.

[kirkbrauer]

@roji Can we merge npgsql/npgsql#6295? I think it should be ready now.

[roji]

@kirkbrauer here's a (likely last) review - this seems almost ready to merge, see the remaining unresolved comments.

[roji]

Just a note that this is still pending.

[roji]

Thanks @kirkbrauer - all looks good, am approving and will merge this for 10.0.

Thanks again for the very high-quality work, and for the patience to work through all my comments! It was a pleasure to review your work and get it merged.

[roji]

@kirkbrauer note the small refinement in #3664, which automatically detects if any property in the EF model uses cube, and automatically installs the extension if so.

Regardless, am finalizing the 10 release. Are you interesting in submitting docs on the various translations cube supports on this page (source)? If you don't like this kinda work no worries at all - just let me know and I'll do it.

[kirkbrauer]

Hi @roji, I took a stab at it, I'll make a PR soon with the docs. I created a new cube.md page as there are a few details or we can just put it in the main translations.md for simplicity.

[roji]

@kirkbrauer I say let's put it in its own section inside translations.md, that's what we do for most other things.