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

ParameterExtractingExpressionVisitor::GetValue #13899

Closed
dmitry-lipetsk opened this issue Nov 6, 2018 · 9 comments
Closed

ParameterExtractingExpressionVisitor::GetValue #13899

dmitry-lipetsk opened this issue Nov 6, 2018 · 9 comments

Comments

@dmitry-lipetsk
Copy link
Contributor

EFCore: master

Could you change

private object GetValue(Expression expression, out string parameterName)

to

protected virtual object GetValue(Expression expression, out string parameterName)

?

My Data Provider for EF needs in overriding of this method for preparing (transforming) expression before local evaluation.

Thanks.

@smitpatel
Copy link
Contributor

Can you describe in detail why you need to override that method?

@dmitry-lipetsk
Copy link
Contributor Author

I already wrote here about this :)

For equal calculation of expression at server and at client side.

For example, string.Substring not allows startIndex>=length_of_sting, but DBMS allows and returns empty string.

DBMS allows calculation the length of NULL-string, string.Length - no.

And other changes.

I remap "problem" nodes of expression (in overrided GetValue) to my functions, which execute by DBMS-rules.

 protected override object GetValue(Expression expression,
                                    out string parameterName)
 {
#if TRACE
  Core.Core_Trace.Method
   ("LcpiOleDb__ParameterExtractingExpressionVisitor::GetValue\n"
   +"  expression: {0}",
    expression);
#endif

  if(!Object.ReferenceEquals(expression,null))
  {
   var prepareSvc
    =Core.Engines.EngineSvcUtils.QuerySvc<Core.Engines.EngineSvc__ExpressionVisitor>
      (m_CnOptions,
       Core.Engines.EngineSvcID.EngineSvc__ExpressionTree__PrepareForLocalEvaluation);

   expression
    =prepareSvc.Visit(expression);

   Debug.Assert(!Object.ReferenceEquals(expression,null));
  }//if

  return base.GetValue
          (expression,
           out parameterName);
 }//GetValue

@smitpatel
Copy link
Contributor

In general there are discrepancy in how C# works vs how DB works. Major difference being string comparison case sensitivity. Further, only DB has concept of null calculations giving back null rather than exception. Though, I am not able to understand how above differences requires interaction from provider side in ParameterExtraction. Can you write query and what is current parameterization and what is parameterization you are trying to get and why?

@dmitry-lipetsk
Copy link
Contributor Author

Hello,

When I write the linq-expression, I expect that all it parts will be executed with equal rules.

For example:

//r.VarCharCol1 (string) contains "qwerty"
//r.VarCharCol2 (string) contains null
string vv1="qwerty";
string vv2=null;
var recs=db.testTable.Where(r => (vv1+vv2).Length==null && (r.VarCharCol1+r.VarCharCol2).Length==null);

For me, "vv1+vv2" and "r.VarCharCol1+r.VarCharCol2" - must returns equal result: NULL.

Though, I am not able to understand how above differences requires interaction from provider side in ParameterExtraction. Can you write query and what is current parameterization and what is parameterization you are trying to get and why?

string vv1="qwerty";
string vv2=null;
var recs=db.testTable.Where(r => (vv1+vv2).Length==null && r.TEST_ID==testID);

By default, EFCore calculates "(vv1+vv2)==null" as FALSE. As result, this query always returns zero number of records.

By DBMS rules "(vv1+vv2)==null" is TRUE. As result, this query returns records.

Evaluation (vv1+vv2) performed in ParameterExtractingExpressionVisitor::GetValue.


I may copy code of ParameterExtractingExpressionVisitor to my provider. But I try to minimize similar imports.

@smitpatel
Copy link
Contributor

Based on definition string.Length returns integer type so comparing SomeString.Length == null is always going to be false. It is true that in DBMS vv1 + vv2 will give null as result because one of the argument is null, but at the same time vv1 + vv2 = null is also null result. We need to generate (vv1 + vv2) is NULL to actually get true result. That is what relational null semantics do in EF Core. Database would generate NULL result for any null argument which C# wouldn't hence we generate special IS NULL statements to compensate for that. With having whole pipeline to deal with that, our philosophy has been that we would actually try to match C# semantics rather than database. And compensate for that when generating SQL. Trying to dictate that parameterization should work like database is counter-intuitive to that philosophy, confusing to users and creates more mismatch in server eval vs client eval.

@dmitry-lipetsk
Copy link
Contributor Author

@smitpatel

Ok, I will continue to use modified sources of EFCore.

At finish (if i get to him) I will return to this problem :)

@ajcvickers
Copy link
Member

Triage: no decision made about behavior or implementation at this stage, but leaving this open to consider in the context of query guiding principals for 3.0 as tracked by #12795

@ajcvickers
Copy link
Member

@smitpatel to bring something to the design meeting.

@smitpatel smitpatel removed this from the 3.0.0 milestone Jan 25, 2019
@smitpatel
Copy link
Contributor

After discussing with @divega
The issue specifically talks about running different implementation of existing C# code in funcletizer. Which is somewhat against our policy to align with C# code. Further, if the client projection includes something like this, it would still use C# semantics rather than custom logic. An extension package would be better choice which rewrites whole expression tree in advance. Also, if a provider does this then it would be some inconsistent across provider how EF Core works.

Removing milestone to discuss in triage.

@smitpatel smitpatel removed their assignment Jan 12, 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
Projects
None yet
Development

No branches or pull requests

3 participants