"Object" aggregator factories mutate queryable data during index merging

[leventov]

When thetaSketch, approxHistogram, hyperUniques and variance metrics columns of several incremental indexes are merged using AggregatorFactory.combine(), it mutates left-hand side object, that could be queried at the same time.

[leventov]

Maybe I can fix this incidentally when I do https://github.com/druid-io/druid/projects/2

[akashdw]

@leventov Current contract of this method does allow for mutation of the input objects and it is callers responsibility to not to use lhs or rhs after calling this method.

[leventov]

@akashdw it's not under caller control to not use those objects during querying. Nevertheless I'm going to fix this, the part is already in #4676, where AggreagateCombiner.combine() does not allow to modify input objects.

[himanshug]

@leventov mutation of lhs is done in thetaSketch to improve performance... if querying them causes any exceptions/bug then that needs to be fixed appropriately in the thetaSketch extension code.
extension writer should be free to mutate the objects, it is creating, if necessary (mostly done for performance )

[leventov]

@himanshug all object aggregators that I listed amend data into lhs. It probably never causes any exceptions, but makes the query results wrong. Extension author could make nothing with it, if he decides to mutate the lhs. The solution in https://github.com/druid-io/druid/projects/2 will be just to retire AggregatorFactory.combine(). AggregateCombiner prohibits mutation, but it doesn't limit performance, because of different design.

[himanshug]

@leventov its fine as long as we don't end up unnecessarily cloning sketches.

[leventov]

@himanshug you can review #4676

[jihoonson]

Hi @leventov, can we close this issue?

[leventov]

@jihoonson rollup aggregation doesn't yet work via makeAggregateCombiner(), so no, this is still an open issue. The current blocker in this process is #5957.

[github-actions]

This issue has been marked as stale due to 280 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If this issue is still
relevant, please simply write any comment. Even if closed, you can still revive the
issue at any time or discuss it on the dev@druid.apache.org list.
Thank you for your contributions.