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

Query: revisit design for function non-determinism and client evaluatability #12672

Closed
divega opened this issue Jul 14, 2018 · 8 comments
Closed
Labels
area-test closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed.

Comments

@divega
Copy link
Contributor

divega commented Jul 14, 2018

Problem

Currently EvaluatableExpressionFilter conflates how we treat two different kinds of functions:

  1. Expressions like DateTime.Now are there because we want to evaluate them on the server to avoid inconsistent results
  2. Expressions like Guid.NewGuid are there because we need to make sure we evaluate them once per row when evaluating in memory

This design leads to potentially incorrect results and severe inefficiencies. E.g. in SQLite, for this query:

    likeToday = await db.Records
        .OrderBy(x => x.Timestamp)
        .Where(x => x.Timestamp.Day == DateTime.Now.AddDays(1).AddDays(-1).Day)
        .FirstOrDefaultAsync();

Desired behavior

For this is case is to obtain the value for DateTime.Now.AddDays(1).AddDays(-1).Day and pass it to a parameter so that it can be compared against x.Timestamp.Day on the server.

Actual behavior

We end up evaluating the whole predicate in-memory.

This is bad because:

  1. We don't really want to move the comparison against x.Timestamp.Day to the client if it can perfectly be evaluated on the server.

  2. As previously stated, we do not want to ever evaluate DateTime.Now on the client to avoid inconsistencies with machine state and time zone differences.

Explanation

Normally we would have translated DateTime.Now to GETDATE() but because it is composed over with functions which (in SQLite) have no server translation, we end up evaluating the whole predicate in-memory. We are doing this only because DateTime.Now is in the black list of "non-deterministic functions" in EvaluatableExpressionFilter which actually implies that it needs to be evaluated once per row. We certainly need that for Guid.NewGuid and Random.Next, but not for DateTime.Now and similar.

DateTime.Now is non-deterministic, but unlike RNG-based functions like Guid.NewGuid() or Random.Next(), calling it has no side-effects on the results of the next call. That is why it is sufficient to call it once per query.

In SQL Server, usage of GETDATE() is a query is treated as a "non-deterministic runtime constant", and is evaluated only once.

Proposal

We should have a different mechanism to keep track of functions that should be evaluated on the server because the client implementation isn't adequate.

This mechanism should work for provider specific and model-mapped functions rather than being hardcoded.

If we cannot evaluate one of these on the server, we should at least issue a client eval warning (which can be turned into an error).

We should not rely on the body of the method throwing. Sometimes the in-memory implementation exists, but isn't adequate.

DateTime.Now (and similar) should not be in the EvaluatableExpressionFilter blacklist because it does not need to be evaluated once per row. Instead, we should use the new mechanism for it.

Ultimately, we can add the ability to issue a separate query to evaluate on the server (tracked by #11466).

For more details, see discussions at:

@pmiddleton
Copy link
Contributor

The fundamental issue is that there is no metadata store for functions and properties which are mapped in relational and by providers.

Currently when we need to translate something we blindly ask all registered providers "hey can you deal with this thing" and see if anyone gives us a response.

We need a querable metadata store which we can then use to make all of the various decisions that have been talking about.

Currently we have the dbfunctions stored in the model. Maybe we can expand on that store to deal with properties and built in mappings. My original implementation of dbfunctions did that for the relational mapped functions so it should be possible to expand on.

@pmiddleton
Copy link
Contributor

We can also look into having all default mappings put into this new store via a convention so that they could be modified at runtime is someone wanted to.

@sdanyliv
Copy link

@divega, i hope our blog post will help you to move in right direction. We almost never have such problems and extend query generator on the fly in minutes.

@divega divega changed the title Query: design change for function non-determinism Query: design change for function non-determinism and server-only evaluation Jul 22, 2018
@divega divega changed the title Query: design change for function non-determinism and server-only evaluation Query: revist design for function non-determinism and server-only evaluatability Jul 22, 2018
@divega divega changed the title Query: revist design for function non-determinism and server-only evaluatability Query: revist design for function non-determinism and client evaluatability Jul 22, 2018
@ajcvickers
Copy link
Member

@divega Curious what would be the expected behavior for the following two queries?

 likeToday = await db.Records
        .OrderBy(x => x.Timestamp)
        .Where(x => x.Timestamp < DateTime.Now)
        .Where(x => x.Timestamp.Day == DateTime.Now.AddDays(1).AddDays(-1).Day)
        .FirstOrDefaultAsync();
 likeToday = await db.Records
        .OrderBy(x => x.Timestamp)
        .Where(x => x.Timestamp < DateTime.Now)
        .FirstOrDefaultAsync();

@smitpatel
Copy link
Contributor

@divega - Can you answer above question?

@ajcvickers
Copy link
Member

ajcvickers commented Jan 23, 2019

Note: based on current plan for 3.0 "the secondfirst example" will throw and hence there is nothing to do here except validate the behavior in 3.0.

@ajcvickers ajcvickers changed the title Query: revist design for function non-determinism and client evaluatability Query: revisit design for function non-determinism and client evaluatability Jan 23, 2019
@smitpatel
Copy link
Contributor

Currently EvaluatableExpressionFilter conflates how we treat two different kinds of functions:
Expressions like DateTime.Now are there because we want to evaluate them on the server to avoid inconsistent results
Expressions like Guid.NewGuid are there because we need to make sure we evaluate them once per row when evaluating in memory

Now that we require full server translation, both of the above merge into same thing since there is no in memory evaluation.
Further, above cases may arise in top level projection (where client eval is allowed) but at that point, we can just pull from server whatever is translated piece is (be it DateTime.Now only).

@divega divega removed this from the 3.0.0 milestone Jun 20, 2019
@divega
Copy link
Contributor Author

divega commented Jun 20, 2019

There is no ambiguity anymore. The expression either translates to the server or throws.

@divega divega closed this as completed Jun 20, 2019
@smitpatel smitpatel removed their assignment Jan 12, 2022
@ajcvickers ajcvickers added the closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. label Mar 10, 2022
@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-test closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed.
Projects
None yet
Development

No branches or pull requests

5 participants