Hi Julian, Thank you for the feedback and good suggestions.
The idea of lazy load is reasonable. However, the testing of this feature is still tricky, because: 1. When we have changed the flag, all subsequent test cases are all also affected (many components of JaninoRelMetadataProvider are declared as static, which is used by all test cases in the same JVM, so it is difficult to revert the changes made by a single test case). 2. All test cases may be run in parallel, so when we change the flag, other test cases running concurrently may fail. (The JaninoRelMetadataProvider object is a singleton, which is shared by all threads). I have updated the PR [1] to implement the idea of lazy load. Could someone please take a look? Best, Liya Fan [1] https://github.com/apache/calcite/pull/1901 On Sat, Sep 19, 2020 at 1:50 AM Julian Hyde <[email protected]> wrote: > 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 > >> >
