Skip to content

Add PushDownDereferencesThroughMarkDistinct rule to optimizer#8962

Merged
losipiuk merged 2 commits intotrinodb:masterfrom
losipiuk:lo/PushDownDereferencesThroughMarkDistinct
Oct 28, 2022
Merged

Add PushDownDereferencesThroughMarkDistinct rule to optimizer#8962
losipiuk merged 2 commits intotrinodb:masterfrom
losipiuk:lo/PushDownDereferencesThroughMarkDistinct

Conversation

@losipiuk
Copy link
Copy Markdown
Member

As spotted by @kasiafi PushDownDereferencesThroughMarkDistinct was missing from the list of rules.
Likely an omission.

Copy link
Copy Markdown
Member

@phd3 phd3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮

Is there an easy-to-add test (i.e. in TestDereferencePushDown) if we've identified issue with a particular query?

I'm not sure what is our general policy for e2e plan tests for optimizer rules, but that could've saved us here. Since the rule has a test, I'm fine with merging this as is.

Copy link
Copy Markdown
Member

@kasiafi kasiafi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phd3 This was identified while inspecting code, so no ready test case. I think it would be worthy to add one.

@mosabua
Copy link
Copy Markdown
Member

mosabua commented Oct 28, 2022

@losipiuk @kasiafi @phd3 is this still valid. Looks like a very quick change that might already be done actually. Could you close or update and merge please?

@losipiuk
Copy link
Copy Markdown
Member Author

@losipiuk @kasiafi @phd3 is this still valid. Looks like a very quick change that might already be done actually. Could you close or update and merge please?

Oh - it was left behind :) Let me see if it is still valid

PushDownDereferencesThroughMarkDistinct was missing from list of
rules.
@losipiuk losipiuk force-pushed the lo/PushDownDereferencesThroughMarkDistinct branch from 8dfd89f to a0e6483 Compare October 28, 2022 08:29
@losipiuk losipiuk merged commit fce6607 into trinodb:master Oct 28, 2022
@github-actions github-actions bot added this to the 402 milestone Oct 28, 2022
@colebow
Copy link
Copy Markdown
Member

colebow commented Nov 2, 2022

Does this have a tangible user impact / does it need a release note?

@losipiuk
Copy link
Copy Markdown
Member Author

losipiuk commented Nov 7, 2022

Does this have a tangible user impact / does it need a release note?

It can impact performance some obscure queries for which pushdown would not happen without hte change. But it is not anything substantial, and also would be super hard to percisly describe it in RN. Let's skip it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants