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

Add Lookup#resolveGroup method and make Lookup#resolve depricated #8698

Closed
wants to merge 1 commit into from

Conversation

sopel39
Copy link
Contributor

@sopel39 sopel39 commented Aug 8, 2017

In the future, equivalence based memo would
store multiple plan nodes per group. Therefore
resolve() that returns singleton should be unused.

This is part of ongoing effort to introduce equivalence based memo and exploratory optimizer: #7169
It is natural step after Support simple pattern matching with captures that makes rule interface compatible with equivalence based memo.

FYI @martint @kokosing

@sopel39
Copy link
Contributor Author

sopel39 commented Aug 8, 2017

@martint your opinion would be appreciated

@kokosing
Copy link
Contributor

kokosing commented Aug 8, 2017

I would refrain with this for a while as I have some doubts if the fact that memo holds multiple nodes per group should be exposed to rules.

Why not to work on traits? This would be helpful even with current memo and rules implementation. We could port QueryCardinalityUtil or DistinctOutputQueryUtil.

Copy link
Contributor

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

As I said, lets wait a bit with this.

@sopel39
Copy link
Contributor Author

sopel39 commented Aug 9, 2017

I would refrain with this for a while as I have some doubts if the fact that memo holds multiple nodes per group should be exposed to rules.

That is the intention of this PR. We already rely on singleton Lookup#resolve a lot and since we have pattern matching we should discourage users from using resolve() by exposing the fact that there might be group of nodes underneath (it is counter-intuitive that GroupReference resolves to just one node). Instead of dropping the bomb and removing Lookup#resolve (which will take some time to do since we need to port rules that depend on it) it is better to make it compatible with equivalence memo and slowly port rules while not adding new rules that use singleton Lookup#resolve.

After this PR lands it opens multiple parallel/independent tasks:

  • model traits so that we can port rules that use singleton Lookup#resolve
    • incorporate traits in iterative optimizer
    • port rules when traits are modeled/implemented
  • implement support for groups in pattern matcher
  • start working on equivalence memo and exploratory optimizer (we want it to implement Memo interface)

I think this PR is next logical step in order to continue optimizer since we slowly want to divert from Lookup#resolve

@@ -28,7 +30,7 @@
* If the node is not a GroupReference, it returns the
* argument as is.
*/
PlanNode resolve(PlanNode node);
List<PlanNode> resolve(PlanNode node);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of changing this method, I would deprecate it and add a new method resolveGroup which would be used to implement this method.

In the future, equivalence based memo would
store multiple plan nodes per group. Therefore
resolve() that returns singleton should be unused.
@sopel39 sopel39 changed the title Make Lookup#resolve return list of nodes Add Lookup#resolveGroup method and make Lookup#resolve depricated Aug 9, 2017
@sopel39
Copy link
Contributor Author

sopel39 commented Aug 9, 2017

applied comment

Copy link
Contributor

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

minor comments

@@ -28,7 +31,20 @@
* If the node is not a GroupReference, it returns the
* argument as is.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

please update javadoc

@@ -76,7 +76,7 @@ public PlanNode optimize(PlanNode plan, Session session, Map<Symbol, Type> types
}

Memo memo = new Memo(idAllocator, plan);
Lookup lookup = Lookup.from(memo::resolve);
Lookup lookup = Lookup.from(planNode -> ImmutableList.of(memo.resolve(planNode)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use Collections.singletonList() instead of ImmutableList.of with single argument. But up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ImmutableList.of() plays better with ImmutableList.copyOf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants