Hi, James. Thanks very much for your kind reply. Actually I am not worrying about the polymorphic calls. And I know that cascading simple if statements works well in some scenarios. I was just wondering whether a bunch of instance-of would be much slower than simple comparisons of integers. Let alone further optimizations that jvm could apply to simple switch-case.
Well, if the performance data shows an acceptable deviation, it shall be ok. Thanks, Jinpeng On Mon, Jan 17, 2022 at 2:44 PM James Starr <[email protected]> wrote: > Cascading if statements such as the ones generated, are generally faster > than polymorphic calls in java. Do you worry about polymorphic calls to > rel nodes? The map look up is at least a magnitudes more expensive than the > dispatch. The key generation for the map look was also a magnitude more > expensive. Even then it is near impossible to measure a difference with > large real world queries that do any meaningful optimization as long as the > metadata system is fairly efficient. Jacques implemented the dispatch with > a map, and his measurements were within a standard deviation. > > James > > On Sun, Jan 16, 2022 at 9:17 PM Jinpeng Wu <[email protected]> wrote: > > > Hi, James. I just noticed the change. And I have a little concern about > > it. > > > > The original implementation uses a switch-case against the class id (an > > integer). So it has to regenerate the handler after a new relnode type > > arrives. It could be bad for some adhoc optimization processes. But it is > > friendly to long-live services who will achieve best performance after > > fully warmed up. > > > > The new design uses a bunch of if-else and instance-of operators. To my > > understanding, those operators are much heavier than a switch-case > against > > an integer value, especially for such hot operations of metadata query. > So > > I wonder whether it will affect the overall latencies. > > > > Thank you. > > Jinpeng Wu > > > > > > On Mon, Jan 17, 2022 at 9:42 AM guangyuan wang <[email protected] > > > > wrote: > > > > > Thank you very much for your answer. > > > > > > James Starr <[email protected]> 于2022年1月17日周一 09:09写道： > > > > > > > That is old code. The generated code depends on knowing all types of > > rel > > > > node, so all the code must be regenerated after discovering a new > type > > or > > > > rel node. Apache calcite main does not have this requirement. > > > > > > > > James > > > > > > > > On Sun, Jan 16, 2022 at 4:35 PM guangyuan wang < > > [email protected] > > > > > > > > wrote: > > > > > > > > > Yes, I can. > > > > > Here is the link: > > > > > > > > > > > > > > > > > > > > https://github.com/apache/calcite/blob/7655bd735f10de5be1eca8bb9af475b3b2ac63b6/core/src/main/java/org/apache/calcite/rel/metadata/JaninoRelMetadataProvider.java#L481 > > > > > > > > > > Jacques Nadeau <[email protected]> 于2022年1月17日周一 03:09写道： > > > > > > > > > > > Can you please provide a url link to the line of code you are > > > referring > > > > > to > > > > > > in github? > > > > > > > > > > > > On Sat, Jan 15, 2022, 5:52 PM guangyuan wang < > > > [email protected] > > > > > > > > > > > wrote: > > > > > > > > > > > > > Thank you very much for your answer. > > > > > > > I'm reading source code these days, I'm a little confused about > > > the " > > > > > > > JaninoRelMetadataProvider.revise()" method. > > > > > > > So I'd like to know the reason why invalidate all of the caches > > of > > > > > > HANDLERS > > > > > > > when adding a new RelNode class to ALL_RELS. > > > > > > > > > > > > > > Jacques Nadeau <[email protected]> 于2022年1月16日周日 02:04写道： > > > > > > > > > > > > > > > I should you produce a test case that represents the specific > > > > concern > > > > > > you > > > > > > > > have as opposed to proposing a snippet of code. I'm not sure > > what > > > > you > > > > > > > > propose is necessary. I think their is implicit expected > logic > > > > that a > > > > > > > > revise should only influence an exception outcome, not a > value > > > > > outcome. > > > > > > > > Only value outcomes are cached so I don't see where there > would > > > be > > > > a > > > > > > > > problem. > > > > > > > > > > > > > > > > If you're revising value outcomes based on revise call (not > > just > > > > > > > exception > > > > > > > > outcomes), I think you're probably breaking the expected > > > contract. > > > > (I > > > > > > say > > > > > > > > think here because I don't think docs make this clear and > > wasn't > > > > the > > > > > > > person > > > > > > > > that wrote the original code.) > > > > > > > > > > > > > > > > On Sat, Jan 15, 2022, 3:23 AM guangyuan wang < > > > > > [email protected] > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Dear community > > > > > > > > > > > > > > > > > > Why should invalidate all of the caches of HANDLERS when > > > adding a > > > > > new > > > > > > > > > RelNode class in JaninoRelMetadataProvider.revise() method? > > > > > > > > > > > > > > > > > > The Codes are below: > > > > > > > > > > > > > > > > > > package org.apache.calcite.rel.metadata > > > > > > > > > > > > > > > > > > public class JaninoRelMetadataProvider implements > > > > > > RelMetadataProvider { > > > > > > > > > > > > > > > > > > synchronized <M extends Metadata, H extends > > > MetadataHandler<M>> H > > > > > > > revise( > > > > > > > > > > > > > > > > > > Class<? extends RelNode> rClass, MetadataDef<M> def) { > > > > > > > > > if (ALL_RELS.add(rClass)) { > > > > > > > > > HANDLERS.invalidateAll(); > > > > > > > > > } > > > > > > > > > //noinspection unchecked > > > > > > > > > return (H) create(def); > > > > > > > > > } > > > > > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
