CALCITE-3901 looks like a reasonable approach. I like the idea that it would throw and tell you the classes and metadata types that you have missed.
The packaging in 3901 isn’t great. I don’t believe that the test needs to read a system property and not even via the CalciteSystemProperty mechanism. We can do better. Maybe we have a class name and arguments for the metadata provider, which is loaded on demand. Will work for the test and also in production. Julian > On Sep 18, 2020, at 12:57 AM, Fan Liya <[email protected]> wrote: > > Hi Enrico, > > We have also observed the problem. > > Janino compilation is time consuming. For each single Java class, it takes > tens of milli-seconds. > Moreover, the compilation will take place multiple times, because: > 1. We have multiple types of metadata. > 2. The class for a particular type of metadata may be regenerated and > recompiled, because a rel node was not registered at first. > > We opened CALCITE-3901 and provided a PR [1] to address problem 2 by > reducing the number of compilations. However, so far it has not been > accepted yet. > > To solve problem 1, I think we have two potential solutions: > 1. Provide a metadata provider that is not based on codegen and Janino > compilation (Obviously, this will be a long term plan). > 2. Save the generated Java class, and compile it with the client code, and > change the implementation of JaninoMetaDataProvider#load3 so that if a > handler can be found in the current class loader, then it skips the codegen > and compilation. > > Best, > Liya Fan > > > > > > > [1] https://github.com/apache/calcite/pull/1901 > > > >> On Fri, Sep 18, 2020 at 3:01 PM Enrico Olivelli <[email protected]> wrote: >> >> Hi, >> I am seeing that JaninoRelMetadataProvider is quite expensive, at least for >> the "boostrap" phase of the system. >> >> It is a cool piece of software and it is working well, but in some cases >> probably it could be quite overkill, for instance in test cases of >> applications that mostly run only one query and then end. >> >> You could argue that the price of the compilation is paid only once and >> there is no need to complicate things for some corner cases that probably >> do not affect production cases. >> >> In CALCITE-3901 we introduced calcite.enable.regenerate.metadata.handler >> system property in order to limit the number of times that Janino kicks in >> but not to drop it at all. >> >> From the code I see that it is a subclass of RelMetadataProvider, but there >> is no way to get rid of it in VolcanoPlanner. >> >> Also, Janino is a third party library, and having the ability of dropping >> it will help in having a smaller set of dependencies downstream (but this >> is not blocker at the moment, it is only a nice-to-have) >> >> Do you have any experience with this problem ? Is there any chance to add >> some configuration option to pass a RelMetadataProvider implementation >> that does not rely on dynamic code generation ? >> If the idea is valuable I will be happy to work on it. >> >> See this issue for reference, with a full stacktrace of the execution >> https://github.com/diennea/herddb/issues/517 >> >> Regards >> Enrico >>
