[Native] - approx_set() has started returning different number of values against c++ workers recently.

[mknegi]

@pramodsatya - I am opening this issue as per your suggestion to find the root cause of this problem with approx_set()

Current Behavior

All the testApproxSet*() testcases in AbstractTestQueries.java were modified earlier as per the new values that were being returned by this function being used along with the cardinality and I remember discussing with you earlier and I was told that there could be variance in the values returned by the prestissimo implementation for this function. But now all these testcases started failing recently in the pipeline. For example this was the original testApproxSetBigint()

    public void testApproxSetBtestApproxSetBigintigint()
    {
        MaterializedResult actual = computeActual("SELECT cardinality(approx_set(custkey)) FROM orders");

        MaterializedResult expected = resultBuilder(getSession(), BIGINT)
                .row(1002L)
                .build();

        assertEquals(actual.getMaterializedRows(), expected.getMaterializedRows());
    }

And it was changed to the following:

    @Test
    public void testApproxSetBigint()
    {
        MaterializedResult actual = computeActual("SELECT cardinality(approx_set(custkey)) FROM orders");

        // PRESTISSIMO_FIX
        MaterializedResult expected = resultBuilder(getSession(), BIGINT)
                .row(990L)
                .build();

        assertEquals(actual.getMaterializedRows(), expected.getMaterializedRows());
    }

And recently it started failing with this difference:

Expected :[990]
Actual   :[1005]

So, now the actual value is closer to the value for the java worker (1002 vs 1005). My question is would this testcase be flaky like this in future as well ?

[tdcmeehan]

I feel like we shouldn't be testing for exact values for approx_set, since it's backed by a HyperLogLog.

[pramodsatya]

I feel like we shouldn't be testing for exact values for approx_set, since it's backed by a HyperLogLog.

Thanks @tdcmeehan, @mknegi shared that the query output changed recently and the query output was consistent between runs so wanted to check what changed in the velox function implementation. Looks like this commit fixes the standard error in velox to match that of Presto: https://github.com/facebookincubator/velox/pull/9399 and accounts for this difference. Closing this issue.