Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translate Guid.NewGuid() #695

Closed
eshangin opened this issue Nov 12, 2018 · 11 comments
Closed

Translate Guid.NewGuid() #695

eshangin opened this issue Nov 12, 2018 · 11 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@eshangin
Copy link

eshangin commented Nov 12, 2018

Hi. Today tried to apply .OrderBy(item => Guid.NewGuid()) to get items in random order but as I see Npgsql unable to translate it to Postgres query. In case of MsSQL it works and translates to order by newid() query.
I think it will be good to have the same kind feature in Npgsql. Maybe it could be in EF.Functions, I'm not sure...

@roji
Copy link
Member

roji commented Nov 12, 2018

Can you please post full details on what happens (exception stack trace, etc.)? Npgsql is definitely supposed to translate Guid.NewGuid()...

@austindrenski
Copy link
Contributor

Some version info would be useful here too (e.g. PostgreSQL, Npgsql provider).

I could see this getting caught up in an evaluatable filter and the ordering being done on the client (i.e. no exception, just log warnings about local eval). But that's just speculation without version info.

@eshangin
Copy link
Author

"PostgreSQL 9.6.10, compiled by Visual C++ build 1800, 64-bit"
We use package https://www.nuget.org/packages/Npgsql.EntityFrameworkCore.PostgreSQL/2.1.2

The code looks like this:

var result = query.OrderBy(item => Guid.NewGuid()).Skip(offset_value).Take(limit_value).ToArray();

There is no exception. As I see it just calls query like select * from "TableName". Then seems it makes order by new guid, skip, take in memory.
But it will be good if random ordering will be done in database using query like select * from "TableName" order by random() limit {limit_value} offset {offset_value}

@austindrenski
Copy link
Contributor

austindrenski commented Nov 13, 2018

@eshangin Thanks for supplying the additional information.

@roji It looks like we don't actually have a method translator for Guid.NewGuid(), and it doesn't look like there's a default implementation from EF Core (e.g. SqlServer has its own implementation in SqlServerNewGuidTranslator).

Am I overlooking something, or should we add the translator?

Design notes

  • Presumably this would map to uuid_generate_v4() from the uuid-ossp extension.
  • Since it's an extension, we wouldn't want it enabled by default, so possible options include:
    • Handle as a plugin package for uuid-ossp.
    • Handle by checking for a model annotation.

@austindrenski austindrenski changed the title Order by random OrderBy Guid.NewGuid() Nov 13, 2018
@austindrenski austindrenski changed the title OrderBy Guid.NewGuid() Translate .OrderBy(_ => Guid.NewGuid()) Nov 13, 2018
@YohDeadfall
Copy link
Contributor

The problem here is in the uuid type which doesn't have random generators out of the box. Therefore, the user should use one of two extensions. We can't dictate which one must be used. It's a documented limitation.

To workaround the issue create an extension you want and a database function. For example:

// Requires pgcrypto
[DbFunction]
public static Guid GenRandomUuid() => throw new NotImplementedException();

@eshangin
Copy link
Author

The goal is to make random ordering. We could use .OrderBy(item => Guid.NewGuid()) with MsSQL and it is translated to order by newid(). For postgress I think we should get order by random() in the result. So maybe is should be one new EF.Functions which will be used like .OrderBy(item => EF.Functions.RandomOrder()) ? I'm not sure at the moment if new function like this could be implemented, just an idea...

@roji
Copy link
Member

roji commented Nov 13, 2018

@austindrenski you're right - seems like we're missing a translator for Guid.NewGuid(). However, I think we can add this as a regular translator, on by default - the fact that something is a PostgreSQL extension doesn't necessarily mean that on the Npgsql side it needs to be supported as a plugin... A good example is the hstore type, which is an extension but is supported out of the box. I think this is OK for extensions which are pretty "standard". One reason I think that a plugin isn't very useful here, is that the user would still have to manually add the uuid-ossp extension on the model (unless I'm mistaken we can't do that for them from a plugin).

If we do add support out-of-the-box, and the user uses Guid.NewGuid() without adding the extension, they'd simply get a PostgreSQL exception about the function not being found, which seems OK to me. If we want to go further, I think that would require the translator to have access to the model, where it could check whether the uuid-ossp was specified or not, and raise a more explicit exception. IMHO it's not really worth it...

As @YohDeadfall says, the user can choose between the two extensions - although from my PostgreSQL experience uuid-ossp is by far more widely-used. Even if we make that supported out-of-the-box, a plugin could still be used to override the translation and use another one.

What do you guys think? I'm not totally against the idea of a plugin, but it seems easier and more natural to just have it in.

@roji
Copy link
Member

roji commented Nov 13, 2018

On an unrelated note, does it make sense to think about translating Math.Random() System.Random() to PostgreSQL random() (instead of a custom EF.Functions.RandomOrder() as @YohDeadfall proposed)? That could be used for random ordering without any complications...

@austindrenski
Copy link
Contributor

Ah that's right, I forgot that we do something similar with hstore. Agreed that a plugin would be overkill for one item here. I can start putting together a PR.

Do you mean System.Random()? I think that's currently excluded by an evaluatable filter, but I do think that would be preferred to adding something on EF.Functions. (Unless there's a compelling reason to keep it excluded, of course.)

@roji
Copy link
Member

roji commented Nov 13, 2018

@austindrenski great, will look forward to your PR.

Of course you're right - I meant System.Random(), and you're right that it's excluded in the filter. The explanation for that is given in dotnet/efcore#12672: if it weren't in the filter, EF Core would evaluate it exactly once and use the same value across all rows. This is because at the moment the concept of "evaluatable" is very coarse, and more nuances are needed (evaluate on server but cache across rows, evaluate on server but don't cache across rows...).

But to go back to the original question: the fact that Math.Random() is excluded by the filter means that it's always server-evaluated, so we can translate it, can't we? After all it's logically identical to generating a random new UUID...

@austindrenski
Copy link
Contributor

the fact that Math.Random() is excluded by the filter means that it's always server-evaluated, so we can translate it, can't we?

Ope, you're right. I had that backwards (again). We should be good to translate it.

@austindrenski austindrenski self-assigned this Nov 13, 2018
@austindrenski austindrenski added the enhancement New feature or request label Nov 13, 2018
@roji roji added this to the 2.2.0 milestone Nov 14, 2018
@roji roji changed the title Translate .OrderBy(_ => Guid.NewGuid()) Translate Guid.NewGuid() Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants