Use deterministic map in equality inference

[sopel39]

No description provided.

[martint]

Why?

[sopel39]

We don't want plan generation to be non-deterministic. Other collections in this class are already deterministic

[martint]

I guess my question is why would this cause the plan to be non-deterministic? Where does the iteration order matter in this method?

As far as I can tell, none of the plan tests are flaky because of this.

[sopel39]

There is enumeration below:

        // For every non-derived expression, extract the sub-expressions and see if they can be rewritten as other expressions. If so,
        // use this new information to update the known equalities.
        Set<Expression> derivedExpressions = new LinkedHashSet<>();
        for (Expression expression : byExpression.keySet()) {

if you have cos(x)=0, x=y, cos(y)=0 then visit order might matter to what will be considered a derivedExpressions expression.

[martint]

Can you add a test that would fail (sporadically) due to this?

[sopel39]

I was thinking about something like:

    @Test(invocationCount = 100)
    public void testDerivedExpressionDeterminism()
    {
        EqualityInference inference = EqualityInference.newInstance(
                metadata,
                equals(nameReference("a"), add("b", "z")),
                equals(nameReference("b"), nameReference("d")),
                equals(nameReference("a"), add("d", "z")));
        // generated equalities should be deterministic
        assertEquals(
                inference.generateEqualitiesPartitionedBy(symbols("a", "b", "z", "d")).getScopeEqualities(),
                ImmutableList.of(equals(nameReference("a"), add("b", "z")), equals(nameReference("b"), nameReference("d"))));
    }

but generateEqualitiesPartitionedBy doesn't filter derived expressions when rewriting so it kind of work.

It might work deterministically with current code, but I'm not 100% sure. Hence I would land it anyway