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: improve optimization of "false==(expr)" #23121

Closed
dmitry-lipetsk opened this issue Oct 28, 2020 · 4 comments · Fixed by #23239
Closed

Query: improve optimization of "false==(expr)" #23121

dmitry-lipetsk opened this issue Oct 28, 2020 · 4 comments · Fixed by #23239
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@dmitry-lipetsk
Copy link
Contributor

Hello,

EFCore: main (no changes in your generation logic).
DataProvider: my own.

I've tested two equal linq expressions and your SQL generator generated two correct, but slightly different SQLs.

I think, your compiler may create two identical SQLs for both cases.

Context

  [Table("TEST_MODIFY_ROW")]
  public sealed class TEST_RECORD
  {
   [Key]
   [Column("TEST_ID")]
   public Int64? TEST_ID { get; set; }

   [Column("COL_VARCHAR_10", TypeName="VARCHAR(10)")]
   public string DATA { get; set; }
  };//class TEST_RECORD

  public DbSet<TEST_RECORD> testTable { get; set; }

TEST1 - (expr)==false
var recs1=db.testTable.Where(r => (!(r.DATA==null))==false && r.TEST_ID==testID);
SQL:

SELECT "t"."TEST_ID", "t"."COL_VARCHAR_10"
FROM "TEST_MODIFY_ROW" AS "t"
WHERE "t"."COL_VARCHAR_10" IS NULL AND ("t"."TEST_ID" = :__testID_0)

Looks OK.

TEST2 - false==(expr)
var recs2=db.testTable.Where(r => false==(!(r.DATA==null)) && r.TEST_ID==testID);
SQL:

SELECT "t"."TEST_ID", "t"."COL_VARCHAR_10"
FROM "TEST_MODIFY_ROW" AS "t"
WHERE NOT ("t"."COL_VARCHAR_10" IS NOT NULL) AND ("t"."TEST_ID" = :__testID_0)

Condition "NOT ("t"."COL_VARCHAR_10" IS NOT NULL)" is correct, but looks a little strange :)

Perhaps this research will be useful for improving EFCore.

@ajcvickers
Copy link
Member

@dmitry-lipetsk Curious, what is the reason for writing these queries in a way that is not at all idiomatic to C#? For example, the first thing I would do if I saw this false==(!(r.DATA==null)) && r.TEST_ID==testID) would be to re-write it as r.DATA == null && r.TEST_ID==testID. Is there some reason to instead write the expression in a convoluted way?

@dmitry-lipetsk
Copy link
Contributor Author

dmitry-lipetsk commented Oct 29, 2020

@dmitry-lipetsk Curious, what is the reason for writing these queries in a way that is not at all idiomatic to C#?..... Is there some reason to instead write the expression in a convoluted way?

Hm....

Google translate of answer:

We bought a Japanese chainsaw for some harsh Siberian lumberjacks.
The lumberjacks gathered in a circle and decided to test it.
They brought her in, gave her a tree.
"Zhik," said the Japanese saw.
"Uh, fuck ..." - said the lumberjacks.
They slipped her a thicker tree. "Vzh-zh-zhik!" - said the saw.
"Wow, shit!" - said the lumberjacks.
They slipped a thick cedar into her. "VZh-Zh-Zh-Zh-Zh-Zh-Zh-ZhIK !!!" - said the saw.
"Wow, fuck !!" - said the lumberjacks.
They slipped her a scrap of iron.
"KRYAK!" - said the saw.
"Yeah, fuck !!!" - the harsh Siberian lumberjacks said reproachfully! And they left to cut the forest with axes ...

@ajcvickers
Copy link
Member

Sir Humphrey: I put it to you, Minister, that you are looking a Trojan horse in the mouth.
Hacker: You mean if we look closely at this gift horse, we'll find it's full of Trojans?
Bernard: Um, if you had looked the Trojan Horse in the mouth, Minister, you would have found Greeks inside. Well, the point is that it was the Greeks who gave the Trojan horse to the Trojans, so technically it wasn't a Trojan horse at all; it was a Greek horse. Hence the tag "timeo Danaos et dona ferentes", which, you will recall, is usually and somewhat inaccurately translated as "beware of Greeks bearing gifts", or doubtless you would have recalled had you not attended the LSE.
Hacker: Yes, well, I'm sure Greek tags are all very well in their way; but can we stick to the point?
Bernard: Sorry, sorry: Greek tags?
Hacker: "Beware of Greeks bearing gifts." I suppose the EEC equivalent would be "Beware of Greeks bearing an olive oil surplus".
Sir Humphrey: Excellent, Minister.
Bernard: No, well, the point is, Minister, that just as the Trojan horse was in fact Greek, what you describe as a Greek tag is in fact Latin. It's obvious, really: the Greeks would never suggest bewaring of themselves, if one can use such a participle (bewaring that is). And it's clearly Latin, not because timeo ends in "-o", because the Greek first person also ends in "-o" – although actually there is a Greek word timao, meaning 'I honour'. But the "-os" ending is a nominative singular termination of a second declension in Greek, and an accusative plural in Latin, of course, though actually Danaos is not only the Greek for 'Greek'; it's also the Latin for 'Greek'. It's very interesting, really.

@ajcvickers
Copy link
Member

@maumar to decide whether to keep this open as a backlog enhancement or not.

@maumar maumar changed the title [research] SQL for "(expr)==false" and for "false==(expr)" Query: improve optimization of "false==(expr)" Nov 9, 2020
maumar added a commit that referenced this issue Nov 9, 2020
When optimizing false == x -> !x we were not running optimization for the Not that we just created, like we do for x == false
@maumar maumar added the type-bug label Nov 9, 2020
@maumar maumar added this to the 6.0.0 milestone Nov 9, 2020
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 9, 2020
@maumar maumar closed this as completed in 700ecb6 Nov 12, 2020
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-preview1 Jan 27, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-preview1, 6.0.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants