[CALCITE-3115] Cannot add JdbcRules which have different JdbcConvention to same VolcanoPlanner's RuleSet. #1397
Conversation
…ame VolcanoPlanner's RuleSet (Wenhui Tang)
|
@zabetak , @danny0405 , @vvysotskyi could you please review to finalize the original PR ? |
| */ | ||
| protected static String getRuleName(Class<?> clazz, JdbcConvention out) { | ||
| return String.format(Locale.ROOT, "%s(out:%s)", clazz.getSimpleName(), out); | ||
| } |
There was a problem hiding this comment.
Just one question, can we make more effort to move this method to super class ConverterRule ? It seems that this is a common pattern, it would be nice if we can unify the naming rules so there is less chance we encounter the similar problem.
There was a problem hiding this comment.
This pattern was chosen by me, because it's similar to the one in ConverterRule constructor:
description == null
? "ConverterRule(in:" + in + ",out:" + out + ")"
: description)
Only difference in my code is that I didn't use in rel trait argument, since for jdbc rules it's the same everywhere. I can move the method to ConverterRule, but then ignoring in argument won't be good I think. So, should I move the method up in the hierarchy with pattern like RuleName(in:" + in + ",out:" + out + ")" ?
There was a problem hiding this comment.
The problem only exists for JdbcConvention, right? Other conventions are singletons. If so, I would be OK if this code stayed in JdbcRules.
There was a problem hiding this comment.
Even though other conventions are singletons, we didn't use it as part of the description, i.e. the ElasticSearchConverter rules [1], i believe they have the same issue.
[1]
There was a problem hiding this comment.
Well, exactly. If there is only one instance of convention C, you can only have one instance of a rule that uses C. But if there are two instances of the rule that use c1 and c2, you have to include c1 and c2 in the name of those rules so that their names are different.
Maybe ElasticSearch convention should NOT be a singleton. I suppose we'll run into the bug when someone wants to run a federated query across more than one ElasticSearch database.
There was a problem hiding this comment.
I checked again and almost every adapter converter rules do not include the convention in their description, maybe we can alway include the convention into the rule description and unify the logic into ConverterRule.
There was a problem hiding this comment.
@danny0405 , @julianhyde please, take a look again whether I correctly understood idea. All converter rules description now will contain in,out rel traits.
- Reverted back randon name generation in JdbcConvention - Unified JdbcConverter rules name creation
There was a problem hiding this comment.
The convention name is not restricted by a pattern as it is the case for the rules. This means that by including the convention name to the rule description we may break existing clients that were working correctly so far. This is not a blocking comment but I wanted to highlight this since it was the main reason that I didn't merge directly the original PR. I guess conventions with weird names are not so common so we could wait till such a problem appears in practice. Leaving the final decision to Danny and Julian.
|
Hi @zabetak, I understand your concerns connected with this change, but I think that it is too unlikely that this change breaks existing clients, since currently if the rule does not have a provided description, it includes |
|
Sorry for replying too late., I have been busy with other things in the past few months so that I have not noticed these comments. @ihuzenko Thanks for you PR, It's good. But I notice that it still not solve the problem that the result of JdbcConvention#toString() maybe not obey the description pattern of the RelOptRule. Since the convention name can be assigned when JdbcSchema is created and the name maybe not valid in a rule description. So if we can not find a way to normalize the convention name, can we throw a prompt to the client? |
|
I'm going to merge this PR the way it is right now. Le'ts treat the remaining problems in a new issue if necessary. Thanks @ihuzenko and @wenhuitang for working on this and sorry for being the one who was stalling this issue. |
…on to same VolcanoPlanner's RuleSet (Wenhui Tang, Igor Guzenko) Close apache#1397 Co-authored-by: Igor Guzenko <ihor.huzenko.igs@gmail.com>
Finalizing the PR .