Jackie-Jiang opened a new pull request, #19117:
URL: https://github.com/apache/pinot/pull/19117
## Summary
`DataSchema._storedColumnDataTypes` is a lazily computed cache, held in a
**non-volatile** field:
```java
private ColumnDataType[] _storedColumnDataTypes;
public ColumnDataType[] getStoredColumnDataTypes() {
ColumnDataType[] storedColumnDataTypes = _storedColumnDataTypes;
if (storedColumnDataTypes == null) {
storedColumnDataTypes = new ColumnDataType[numColumns];
for (...) { storedColumnDataTypes[i] =
_columnDataTypes[i].getStoredType(); }
_storedColumnDataTypes = storedColumnDataTypes; // unsafe publication
}
return storedColumnDataTypes;
}
```
The array contents are published unsafely. Nothing establishes a
happens-before edge between the element stores in the
computing thread and the element loads in another thread, so a racing thread
can observe the **non-null array reference
while still reading `null` for its elements**.
That `null` stored type then flows into code that switches on it. `switch`
over an enum compiles to
`$SwitchMap[storedType.ordinal()]`, so the failure surfaces as:
```
Cannot invoke
"org.apache.pinot.common.utils.DataSchema$ColumnDataType.ordinal()" because
"storedType" is null
```
The two frames whose parameter is literally named `storedType`, both fed
from `getStoredColumnDataTypes()`:
- `TypeUtils.convert(Object value, ColumnDataType storedType)`
- `DataBlockExtractUtils.extractValue(DataBlock, ColumnDataType storedType,
int, int)`
`ColumnDataType.getStoredType()` can never return `null` (every constant
sets its stored type to itself or to an
earlier constant) and plan deserialization throws on unknown types rather
than yielding `null`, so a null array element
is the only way to reach this state.
### Why the schemas are shared across threads
The `DataSchema` instances hanging off plan nodes are read concurrently:
- `QueryServer.submitInternal` deserializes a stage plan **once per stage**,
then passes that same instance to
`processQuery` for every `WorkerMetadata` of the stage.
- `QueryDispatcher` builds a *list* of worker ids per server, so a server
routinely runs several workers of the same
stage.
- `QueryRunner.processQuery` runs each worker via
`CompletableFuture.runAsync` on its own thread.
All those workers build their operator trees from the same plan nodes, so
they hit the lazy initialization at the same
moment, deep inside query execution. Affected call sites include
`MultistageGroupByExecutor`,
`MultistageAggregationExecutor`, `WindowAggregateOperator` and
`LeafOperator`.
### Fix
Make the field `volatile`, which supplies the missing happens-before edge.
The racy-single-check idiom is kept intentionally: two threads may each
compute the array, but they compute identical
values, so the duplicate work is harmless.
### Why not compute it eagerly in the constructor
That would remove the `volatile` read and the branch, but it is the wrong
trade: most `DataSchema` instances never need
stored types. One is created per `RelNode` during planning
(`RelToPlanNodeConverter`, `PRelToPlanNodeConverter`), and
there is not a single `getStoredColumnDataTypes()` call anywhere in
`pinot-broker` or `pinot-query-planner` — those
schemas are serialized to proto and dropped. On the server, only the
aggregate / window / leaf / send paths ask; filter,
project, sort, set-op and mailbox-receive schemas never do. Eager
computation would allocate and fill an array for every
plan node of every query to serve a minority of them, and would also move
any failure on a malformed schema from first
use to construction time.
### Overhead
Negligible. All 25 call sites already hoist the array into a local before
looping, so the read happens once per block /
table / results-block rather than once per row. On x86-64 a volatile read of
a reference field compiles to a plain
`mov`; on AArch64 it is `ldar` instead of `ldr`. The volatile write happens
at most a couple of times per `DataSchema`.
Also converts the touched Javadoc to Markdown and documents the
thread-safety contract. The
`org.apache.pinot.common.datatable.DataTable` import is dropped because its
only use was the `{@link}` in the converted
class Javadoc, and checkstyle's `UnusedImports` does not recognize Markdown
links.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
