[
https://issues.apache.org/jira/browse/CALCITE-3947?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17093848#comment-17093848
]
Botong Huang commented on CALCITE-3947:
---------------------------------------
Thanks [~julianhyde] for the detailed comment! Let me elaborate a bit and see
if it makes sense to you.
Firstly, the point of this patch is to make sure the behavior of the planner is
identical across runs for easy debugging. Existing code did try to fulfill this
purpose, that's why lots of LinkedHashSet/Map are used in the planner already.
I simply found a leak in AbstractRelOptPlanner.classes, now a HashSet, whose
iteration order does impact the order rules are matched and fired (see
description for details), and thus making the behavior underterministic.
Secondly, yes keeping it sorted, say with TreeSet, serves the purpose as well.
Considering TreeSet is more costly than LinkedHashSet, and that currently
there's no additional benefit of keeping it sorted for now, I think
LinkedHashSet is a better choice.
> AbstractRelOptPlanner.classes should be LinkedHashSet so that rule match
> order is deterministic across runs
> -----------------------------------------------------------------------------------------------------------
>
> Key: CALCITE-3947
> URL: https://issues.apache.org/jira/browse/CALCITE-3947
> Project: Calcite
> Issue Type: Improvement
> Reporter: Botong Huang
> Priority: Minor
> Time Spent: 10m
> Remaining Estimate: 0h
>
> AbstractRelOptPlanner.classes is used by subClasses() to determine things to
> put into VolcanoPlanner.classOperands, which is then used in
> VolcanoPlanner.fireRules(). Since AbstractRelOptPlanner.classes is now a
> HashSet, its iteration order is not deterministic across runs, making
> debugging hard. It should be LinkedHashSet just like many other fields in the
> planner.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
