Jackie-Jiang commented on code in PR #19117:
URL: https://github.com/apache/pinot/pull/19117#discussion_r3677040787
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -57,14 +56,23 @@
import static java.nio.charset.StandardCharsets.UTF_8;
-/**
- * The <code>DataSchema</code> class describes the schema of {@link DataTable}.
- */
+/// Describes the schema of a [org.apache.pinot.common.datatable.DataTable].
+///
+/// Instances are effectively immutable and safe to share across threads.
Sharing is common: in the multi-stage engine
+/// a stage plan is deserialized once per stage and then handed to every
worker of that stage, so the `DataSchema` of
+/// each plan node is read concurrently by all worker threads on the server.
Review Comment:
Markdown documentation comments (JEP 467) are a permanent feature since JDK
23, and this repo builds on JDK 25 — `pom.xml` sets
`<jdk.version>25</jdk.version>`, which feeds
`maven.compiler.release/source/target`. (The Java 11 bytecode target for the
client/SPI artifacts is a separate setting and doesn't affect the JDK that runs
javadoc.)
`///` is also already established style here rather than something new:
there are ~955 `///` documentation comments in the `pinot-common`,
`pinot-core`, `pinot-spi` and `pinot-query-runtime` main sources.
On the Checkstyle point: the only Javadoc module configured in
`config/checkstyle.xml` is `JavadocStyle` with `checkFirstSentence=false`.
There is no
`MissingJavadocType`/`MissingJavadocMethod`/`JavadocType`/`JavadocMethod`, so
nothing requires a `/** ... */` block. No change needed.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -57,14 +56,23 @@
import static java.nio.charset.StandardCharsets.UTF_8;
-/**
- * The <code>DataSchema</code> class describes the schema of {@link DataTable}.
- */
+/// Describes the schema of a [org.apache.pinot.common.datatable.DataTable].
+///
+/// Instances are effectively immutable and safe to share across threads.
Sharing is common: in the multi-stage engine
+/// a stage plan is deserialized once per stage and then handed to every
worker of that stage, so the `DataSchema` of
+/// each plan node is read concurrently by all worker threads on the server.
@JsonPropertyOrder({"columnNames", "columnDataTypes"})
public class DataSchema {
private final String[] _columnNames;
private final ColumnDataType[] _columnDataTypes;
- private ColumnDataType[] _storedColumnDataTypes;
+
+ /// Lazily computed cache of [#getStoredColumnDataTypes].
+ ///
+ /// `volatile` is required, not just for the null check in
[#getStoredColumnDataTypes]: without it the array
+ /// contents are published unsafely, and a racing thread can read the
non-null array reference while still seeing
+ /// `null` for its elements. Downstream code (e.g. `TypeUtils.convert`,
`DataBlockExtractUtils.extractValue`) then
+ /// switches on a `null` stored type and fails with a `NullPointerException`.
Review Comment:
This is the intended JEP 467 syntax. In Markdown documentation comments, a
CommonMark reference link whose label is a Java reference — `[DataTable]`,
`[#getStoredColumnDataTypes]`, `[Type#method]` — is resolved exactly as `{@link
...}` would be; `{@link}` is not required and is in fact discouraged inside
`///` comments.
The fully-qualified form is used for
`[org.apache.pinot.common.datatable.DataTable]` deliberately: that was
previously `{@link DataTable}`, and since Checkstyle's `UnusedImports` does not
recognise Markdown links, keeping a short `[DataTable]` link would have left
the `DataTable` import looking unused. The import was therefore dropped and the
reference fully qualified so it still resolves.
Also worth noting the anchored line here contains no link syntax at all. No
change needed.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -101,9 +109,11 @@ public ColumnDataType[] getColumnDataTypes() {
return _columnDataTypes;
}
- /**
- * Lazy compute the _storeColumnDataTypes field.
- */
+ /// Returns the stored type of each column, lazily computing and caching it
on first access.
+ ///
+ /// Uses the racy-single-check idiom: two threads may each compute the
array, but both compute the same values, so
+ /// the duplicate work is harmless. Correctness relies on
`_storedColumnDataTypes` being `volatile`; see the field
+ /// for why.
Review Comment:
Same as the sibling comment: `[DataTable]` / `[#method]` / `[Type#method]`
reference links are the JEP 467 replacement for `{@link}` inside `///` comments
and resolve identically.
This particular line (`/// for why.`) contains no link — the reference on
the nearby line is `` `_storedColumnDataTypes` `` in backticks rather than a
link, precisely because linking a private field from a public method's
documentation would not resolve. No change needed.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -57,14 +56,23 @@
import static java.nio.charset.StandardCharsets.UTF_8;
-/**
- * The <code>DataSchema</code> class describes the schema of {@link DataTable}.
- */
+/// Describes the schema of a [org.apache.pinot.common.datatable.DataTable].
+///
+/// Instances are effectively immutable and safe to share across threads.
Sharing is common: in the multi-stage engine
Review Comment:
Good catch, you're right — that wording was an overclaim. `getColumnNames()`
and `getColumnDataTypes()` hand out the internal arrays uncopied, so the
contents are mutable and `BaseGapfillProcessor` does exactly that. Reworded the
class Javadoc to say so explicitly and to scope the thread-safety statement to
read-only sharing.
And to your follow-up: I checked whether the mutability reaches the data
types, since an in-place write to `_columnDataTypes` after the cache is
populated would leave `_storedColumnDataTypes` stale and make this fix
incomplete. Grepping both `getColumnDataTypes()[...] =` and
`getStoredColumnDataTypes()[...] =` across the repo returns no assignments (the
`TimeSeriesBlockSerde` hits are `==` comparisons). So the mutation really is
confined to column names and the cache cannot go stale.
--
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]
