apache
[CALCITE-3901] Introduce an option to disable metadata handler regeneration
According to our investigation, Calcite can produce large performance overhead due to meta data handler regeneration.
To illustrate, suppose we want to get the average size of a rel node, by calling the RelMetadataQuery#getAverageRowSize method. If the rel node type has not been registered before, JaninoRelMetadataProvider#revise method will be called, which will regerenate and recompile the code for the metadata handler.
This process is time-consuming, as compiling code by Janino is time-consuming: It takes more than 10ms to compile a single source file, even if the file is small. According to our investigation, performance overhead due to this problem can be over 10% of the total optimization time.
Therefore, we introduce an option to disable metadata handler regeneration. This forces the client to register all rel node types before calling the metadata service, so as to avoid performance being stolen silently.
For some scenarios, the client mistakenly believes they have registered all rel node types, and this option helps them find the types they have missed.
Thank you @liyafan82 This implementation will unblock this issue on HerdDB ! https://github.com/diennea/herddb/issues/517
@amitvc please follow this fix and ensure we leverage it as soon as it is delivered to a Calcite release
This forces the client to register all rel node types before calling the metadata service
How ?
This forces the client to register all rel node types before calling the metadata service
How ?
If the client fails to register all node types beforehand (possibly by mistake), Calcite will throw an exception, which forces the client to examine their code to register missing node types.
This forces the client to register all rel node types before calling the metadata service
How ?
If the client fails to register all node types beforehand (possibly by mistake), Calcite will throw an exception, which forces the client to examine their code to register missing node types.
I think the behavior you propose make a design violation that we want to initialize the handlers lazily. In current code, we only initialize the handler when it is really necessary (a try catch finally block), but with your change, the initialization changes to be eagerly.
This forces the client to register all rel node types before calling the metadata service
How ?
If the client fails to register all node types beforehand (possibly by mistake), Calcite will throw an exception, which forces the client to examine their code to register missing node types.
I think the behavior you propose make a design violation that we want to initialize the handlers lazily. In current code, we only initialize the handler when it is really necessary (a try catch finally block), but with your change, the initialization changes to be eagerly.
I can understand your concern. The lazy initialization design is reasonable, and our change does not violate this design.
In particular, the handlers are generated only when required. It just makes sure the generation happens only once (if the flag is set).
AFAIK, the custom node types for down-stream projects should usually be few, so how much performance could we gain to encourage an eagerly initialization for all nodes ?
If the promotion is little, i would prefer less config options especially this flag is just throws exception.
AFAIK, the custom node types for down-stream projects should usually be few, so how much performance could we gain to encourage an eagerly initialization for all nodes ?
If the promotion is little, i would prefer less config options especially this flag is just throws exception.
As discussed in the description, the performance cost due to this problem is notable.
Let me illustrate with a concrete example. If we have missed one type of relnode, then when we need to estimate its average column size, we need to regenerate the sizeHandler, which takes 10+ms; then we estimates the row count, and we need to regenerate the rowCountHandler, which takes another 10+ms; and then we need to estimate the selectivity, another 10+ms, and so on.
Please note that, the total optimization time (for Volcano planner) is only about several hundred ms. So the performance penalty can be very expensive.
Without this flag, performance diagnostic can be very difficult. And this issue can only be solved in Calcite (due to lazy initialization, the handler is first generated in the revise method, so it cannot be solved in client code)
Finally, please note that the change does not introduce eagerly initialization.
@julianhyde I have updated the PR according to your suggestions in the ML. Could you please take a look?
Could we expand the revise call to take a RelMetadataProvider as an argument. Then create a static variable using an interface with the revise call in RelMetadataQueryBase. This would allow the removal of RelMetadataQueryBase.THREAD_PROVIDERS and allow customization by projects on how the dispatch is generated without forcing individual implementation into calcite main.
Something like this https://github.com/apache/calcite/pull/2367.
I am in the progress of migrating my company internal branch of Calcite to use the Janino backed metadata provider. However, there are several difficult to work around opinions in the current Janino Metadata Provider we do not share.
Could we expand the revise call to take a RelMetadataProvider as an argument. Then create a static variable using an interface with the revise call in RelMetadataQueryBase. This would allow the removal of RelMetadataQueryBase.THREAD_PROVIDERS and allow customization by projects on how the dispatch is generated without forcing individual implementation into calcite main.
Something like this #2367.
I am in the progress of migrating my company internal branch of Calcite to use the Janino backed metadata provider. However, there are several difficult to work around opinions in the current Janino Metadata Provider we do not share.
@jamesstarr Thanks a lot for your feedback. I took a first pass of #2367 , and it seems to be able to resolve the problem of metadata regeneration?
Could we expand the revise call to take a RelMetadataProvider as an argument. Then create a static variable using an interface with the revise call in RelMetadataQueryBase. This would allow the removal of RelMetadataQueryBase.THREAD_PROVIDERS and allow customization by projects on how the dispatch is generated without forcing individual implementation into calcite main. Something like this #2367. I am in the progress of migrating my company internal branch of Calcite to use the Janino backed metadata provider. However, there are several difficult to work around opinions in the current Janino Metadata Provider we do not share.
@jamesstarr Thanks a lot for your feedback. I took a first pass of #2367 , and it seems to be able to resolve the problem of metadata regeneration?
Currently, Calcite assumes that all custom rel nodes are registered with the JaninoRelMetadataProvider so the handlers do not have to be regenerated. That is the API for calcites metadata. You want to either add check, or generate code at compile time to ensure this is the case. The above changes allows you to do either in your own code base with out having to justify why. Maybe it should be changed in calcite main too, but as this PR is almost a year old, I am not sure it worth the effort.