[CALCITE-5277] Make EnumerableRelImplementor stashedParameters order deterministic to increase BINDABLE_CACHE hit rate

• Type: Improvement
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 1.32.0
• Component/s: core
• Labels: pull-request-available
• Assignee: Ruben Q L
• Reporter: Ruben Q L
• Created: 08/Sep/22 09:06
• Updated: 10/Sep/22 08:40
• Resolved: 08/Sep/22 18:56

Description

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.

    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)));

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).

Issue Links

• links to: GitHub Pull Request #2899