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 <wjpabc...@gmail.com> 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 <wangguangy...@apache.org> > wrote: > > > Thank you very much for your answer. > > > > James Starr <jamesst...@gmail.com> 于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 < > wangguangy...@apache.org > > > > > > 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 <jacq...@apache.org> 于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 < > > wangguangy...@apache.org > > > > > > > > > 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 <jacq...@apache.org> 于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 < > > > > wangguangy...@apache.org > > > > > > > > > > > > > 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); > > > > > > > > } > > > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
