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

[CALCITE-4003] Disallow cross convention matching in TransformationRule #1986

Merged
merged 1 commit into from Jun 10, 2020
Merged

[CALCITE-4003] Disallow cross convention matching in TransformationRule #1986

merged 1 commit into from Jun 10, 2020

Conversation

hsyuan
Copy link
Member

@hsyuan hsyuan commented May 20, 2020

volcanoPlanner.ensureRegistered(rel, rels[0]);
RelSubset subset = volcanoPlanner.ensureRegistered(rel, rels[0]);
// The subset is not used, but we need it, just for debugging
assert subset != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: why not use checkArgument or something similar from Guava? Such that it can both check expected result, stop execute when not satisfying, and provide detailed description.

Copy link
Contributor

Choose a reason for hiding this comment

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

Saying this just because the comment says this assert is for debugging, so I assumed some context will be more useful for people who don't have know this much.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not intended to check null or not, just to silence the IDE that the 'subset' is used. But in fact it is useful to add a breakpoint there to see the content of subset.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. Now I see it is to suppress IDE, style checker, etc.

Thanks for clarification.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find it confusing to use an assertion just to silence some warnings. Usually there are annotations for this kind of things. Apart from that if the content is useful for debugging then why not logging it for real.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, we can use annotations instead.
But logging is not what I want, I just want to add a debug point, check the returned subset for a specific rule match. Logging will print a lot of subsets for all the rule matches that I am not interested in.

Copy link

Choose a reason for hiding this comment

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

after udpate, this exception happend, this exception should not break OPTIMIZE process, JoinAssociateRule enable match EnumerableNestedLoopJoin. java.lang.RuntimeException: rel#3291:EnumerableHashJoin.ENUMERABLE.[](left=RelSubset#88,right=EnumerableNestedLoopJoin#3290,condition==($0, $3),joinType=inner) is a PhysicalNode, which is not allowed in JoinAssociateRule

@hsyuan hsyuan changed the title [CALCITE-4003] Disallow cross convention matching and generation in TransformationRule [CALCITE-4003] Disallow cross convention matching in TransformationRule May 22, 2020
@hsyuan hsyuan merged commit 0c8d0fe into apache:master Jun 10, 2020
@hsyuan hsyuan deleted the CALCITE-4003 branch June 10, 2020 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants