Reuse IndexedInts returned from DimensionSelector.getRow() implementations #5172
Conversation
leventov
changed the title
|
@gianm could you please review this? |
|
I'll also review this PR. |
|
@jihoonson thanks |
| * {@link io.druid.query.aggregation.AggregateCombiner#fold} should be prepared for that and not storing the object | ||
| * returned from this method in their state, assuming that the object will remain unchanged even when the position of | ||
| * the selector changes. This may not be the case. | ||
| */ |
There was a problem hiding this comment.
Looks like this new contract might this break SelectQuery as there we just call getObject and put that object in an event ?
https://github.com/druid-io/druid/blob/535ec437e925effee920e5643277fde2a1b69175/processing/src/main/java/io/druid/query/select/SelectQueryEngine.java#L313
There was a problem hiding this comment.
Thanks for this finding. I think I will need to add a method like getObjectNotReusable() to BaseObjectColumnValueSelector and use it in SelectQueryEngine in one of the future PRs, where object reuse is actually implemented.
There was a problem hiding this comment.
- I am worried about changing the api semantics for existing apis in a backwards incompatible way. In this case if any extensions are using this api, they might break without any compilation error. I think a better way might be to add another api which returns some Holder object.
- Also, IMO api that reuse objects internally are a bit tricky and are quite fragile to implement. Another way could be to use Druid Sequence or a similar api which do not expose the underlying object and provide a way to accumulate or apply computations to the object.
There was a problem hiding this comment.
- This is fair, while developing the actual PR that makes getObject() to return reusable objects I came to the same conclusion, and decided to also rename the "main" method to getReusableObject(), and the second is getNotReusableObject().
Using any "holder" interlayer won't allow to bypass the fact that we inherently going to reuse the same, reusable objects. So I don't see the point of adding such.
There was a problem hiding this comment.
- Implementors of custom
ObjectStrategycould always retreat to use not reusable objects, it just won't be that efficient when a column with theirObjectStrategyis used. Users of this API could also retreat to always usegetNotReusableObject()if they are unsure.
I don't say that introduction of object reusability doesn't add complexity, it does, but there are no interfaces that allow to hide this complexity, Sequence, ColumnValueSelector or any else, because it's still about making immutable objects mutable. Not to say that rewriting everything from ColumnValueSelector to Sequence would be a MASSIVE refactoring and it's absolutely not guaranteed that it will make anything clearer. So it doesn't seem realistic to me.
There was a problem hiding this comment.
In this PR, I removed this doc update of getObject(), because it's premature, this PR is about IndexedInts and DimensionSelectors. Let's focus on it.
|
@nishantmonu51 do you have further comments? |
|
@nishantmonu51 could you please comment? |
|
@nishantmonu51 Thanks for review. Opened #5267 |
This PR starts the course towards reusable objects returned
ColumnValueSelector.getXxx(). This PR makesIndexedIntsreturned from allDimensionSelector.getRow()implementations being reused.This is essential for #4622, but could also reduce garbage creation during querying.