[ 
https://issues.apache.org/jira/browse/CALCITE-5277?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17602193#comment-17602193
 ] 

Thomas Rebele commented on CALCITE-5277:
----------------------------------------

The map itself is not part of the cache key, but it influences the order of the 
statements of the generated code. The generated code is then used as cache key.

The method EnumerableRelImplementor.stash(T, Class<? super T>) modifies the 
map, so it cannot be an ImmutableMap.

> Make EnumerableRelImplementor stashedParameters order deterministic to 
> increase BINDABLE_CACHE hit rate
> -------------------------------------------------------------------------------------------------------
>
>                 Key: CALCITE-5277
>                 URL: https://issues.apache.org/jira/browse/CALCITE-5277
>             Project: Calcite
>          Issue Type: Improvement
>          Components: core
>            Reporter: Ruben Q L
>            Assignee: Ruben Q L
>            Priority: Major
>              Labels: pull-request-available
>             Fix For: 1.32.0
>
>          Time Spent: 50m
>  Remaining Estimate: 0h
>
> Currently, EnumerableRelImplementor stashedParameters is a map with no 
> deterministic order when its values are traversed.
> Because of this, it is possible to have two EnumerableRel which are 
> effectively equal, but which generate a different dynamic code, just because 
> the statements in which stashedParameters are defined are the same, but in 
> different order.
> {code}
>     final Collection<Statement> stashed =
>         Collections2.transform(stashedParameters.values(),
>             input -> Expressions.declare(Modifier.FINAL, input,
>                 Expressions.convert_(
>                     Expressions.call(DataContext.ROOT,
>                         BuiltInMethod.DATA_CONTEXT_GET.method,
>                         Expressions.constant(input.name)),
>                     input.type)));
> {code}
> This has a disadvantage: in this situation, EnumerableInterpretable's 
> BINDABLE_CACHE will not have a cache hit, so the Bindable will need to be 
> unnecessarily recreated in EnumerableInterpretable#getBindable.
> If we guarantee a deterministic order in EnumerableRelImplementor 
> stashedParameters (e.g. insertion order), we would have a cache hit in these 
> situations (and we could skip the bindable re-creation).



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Reply via email to