IncrementalIndex's `in` ThreadLocal doesn't need to be instance-level
- Dominant language
- Java
- Stars
- 14.1k
- Forks
- 3.8k
- Avg merge
- 2d 58m
- Merged PRs (30d)
- 233
Description
`IncrementalIndex`'s `ThreadLocal in` can be `static final`. This `ThreadLocal` doesn't store anything *specific to a given `IncrementalIndex`*: rather, it's used to set a `Row` temporarily, and there is no situation when an `IncrementalIndex` could call recursively into another `IncrementalIndex`, so this temporary assignment cannot be disturbed.
As a safety net, we could add a check `if (in.get() != null) throw ISE(..); in.set(row);`
Furthermore, it would be ideal to get rid of `ThreadLocal` altogether (which would be beneficial for performance, because every `Aggregator` wouldn't need to query `ThreadLocal` upon every indexed row (`ThreadLocal.get()` is like `Map.get()`, not super cheap), and for reducing complexity, but this requires a significant refactoring of `Aggregator` interface and/or indexing logic, so saving this idea for a better time.
Contributor guide
Research direction
Start by locating IncrementalIndex's instance-level ThreadLocal in the indexing path and inspect how Aggregator accesses it for each indexed row. Confirm that the temporary Row value is not shared across recursive IncrementalIndex calls, then make the ThreadLocal static final and add the proposed safety check if appropriate. The broader ThreadLocal removal is explicitly deferred.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- data, databases
- Issue type
- Refactor
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100