This is an automated email from the ASF dual-hosted git repository.
Jackie-Jiang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new c5f81e3af15 Safely publish lazily-initialized caches shared across
threads (#19137)
c5f81e3af15 is described below
commit c5f81e3af15208fdd07d02af3ea6d9350c20eb76
Author: Xiaotian (Jackie) Jiang <[email protected]>
AuthorDate: Fri Jul 31 17:44:16 2026 -0700
Safely publish lazily-initialized caches shared across threads (#19137)
---
.../common/request/context/LiteralContext.java | 22 +++++++++------
.../request/context/predicate/BaseInPredicate.java | 29 +++++++++++++-------
.../context/predicate/RegexpLikePredicate.java | 27 +++++++++++++++----
.../pinot/common/utils/regex/JavaUtilPattern.java | 2 +-
.../pinot/common/utils/regex/Re2jPattern.java | 2 +-
.../pinot/segment/spi/index/AbstractIndexType.java | 31 ++++++++++++++++------
.../apache/pinot/spi/data/DateTimeFieldSpec.java | 9 +++++--
7 files changed, 87 insertions(+), 35 deletions(-)
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java
b/pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java
index 0dc08747195..744cdca228f 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java
@@ -44,14 +44,20 @@ public class LiteralContext {
/// This is used for type conversion, and is not included in {@link #equals}
and {@link #hashCode}.
private final PinotDataType _pinotDataType;
- private Boolean _booleanValue;
- private Integer _intValue;
- private Long _longValue;
- private Float _floatValue;
- private Double _doubleValue;
- private BigDecimal _bigDecimalValue;
- private String _stringValue;
- private byte[] _bytesValue;
+ /// Lazily converted caches of the value, one per stored type.
+ ///
+ /// `volatile` is required, not just for the null checks in the getters: a
literal belongs to the query's expression
+ /// tree, which is built once per query and then read concurrently by the
threads that build and run the per-segment
+ /// plans. Without it the values are published unsafely, and a racing thread
can read a non-null reference whose
+ /// contents are not yet visible to it.
+ private volatile Boolean _booleanValue;
+ private volatile Integer _intValue;
+ private volatile Long _longValue;
+ private volatile Float _floatValue;
+ private volatile Double _doubleValue;
+ private volatile BigDecimal _bigDecimalValue;
+ private volatile String _stringValue;
+ private volatile byte[] _bytesValue;
public LiteralContext(Literal literal) {
switch (literal.getSetField()) {
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/request/context/predicate/BaseInPredicate.java
b/pinot-common/src/main/java/org/apache/pinot/common/request/context/predicate/BaseInPredicate.java
index 926dd42bda3..0bbe77c14fe 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/request/context/predicate/BaseInPredicate.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/request/context/predicate/BaseInPredicate.java
@@ -27,19 +27,28 @@ import org.apache.pinot.spi.utils.BytesUtils;
import org.apache.pinot.spi.utils.TimestampUtils;
-/// Base predicate for IN and NOT_IN.
+/// Base predicate for `IN` and `NOT_IN`.
+///
+/// Instances are read concurrently: a predicate belongs to the query's filter
tree, which is built once per query and
+/// then shared by the threads that build and run the per-segment plans. The
lazily parsed value caches are therefore
+/// published safely; see the fields.
public abstract class BaseInPredicate extends BasePredicate {
protected final List<String> _values;
- // Cache the parsed values
- private int[] _intValues;
- private long[] _longValues;
- private float[] _floatValues;
- private double[] _doubleValues;
- private BigDecimal[] _bigDecimalValues;
- private int[] _booleanValues;
- private long[] _timestampValues;
- private ByteArray[] _bytesValues;
+ /// Lazily parsed caches of the values, one per stored type.
+ ///
+ /// `volatile` is required, not just for the null checks in the getters: a
predicate is read concurrently (see the
+ /// class javadoc), and without it the arrays are published unsafely. A
racing thread can then read the non-null
+ /// array reference while the element writes are still invisible to it,
seeing `0` for the primitive arrays and
+ /// `null` for the object arrays — which silently matches the wrong rows
rather than failing.
+ private volatile int[] _intValues;
+ private volatile long[] _longValues;
+ private volatile float[] _floatValues;
+ private volatile double[] _doubleValues;
+ private volatile BigDecimal[] _bigDecimalValues;
+ private volatile int[] _booleanValues;
+ private volatile long[] _timestampValues;
+ private volatile ByteArray[] _bytesValues;
public BaseInPredicate(ExpressionContext lhs, List<String> values) {
super(lhs);
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/request/context/predicate/RegexpLikePredicate.java
b/pinot-common/src/main/java/org/apache/pinot/common/request/context/predicate/RegexpLikePredicate.java
index d6167b2add1..e7ca7566f4b 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/request/context/predicate/RegexpLikePredicate.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/request/context/predicate/RegexpLikePredicate.java
@@ -24,11 +24,22 @@ import
org.apache.pinot.common.utils.RegexpPatternConverterUtils;
import org.apache.pinot.common.utils.regex.Pattern;
import org.apache.pinot.common.utils.regex.PatternFactory;
-/// Predicate for REGEXP_LIKE with optional match parameters
+/// Predicate for `REGEXP_LIKE` with optional match parameters.
+///
+/// Instances are read concurrently: a predicate belongs to the query's filter
tree, which is built once per query and
+/// then shared by the threads that build and run the per-segment plans. The
lazily compiled pattern is therefore
+/// published safely; see [#getPattern].
public class RegexpLikePredicate extends BasePredicate {
private final String _value;
private final boolean _caseInsensitive;
- private Pattern _pattern = null;
+
+ /// Lazily compiled cache of [#getPattern].
+ ///
+ /// `volatile` is required, not just for the null check in [#getPattern]:
without it the pattern is published
+ /// unsafely, and a racing thread can read the non-null reference while the
fields written by the pattern's
+ /// construction are still invisible to it. Using such a pattern to create a
matcher fails with a
+ /// `NullPointerException`.
+ private volatile Pattern _pattern;
public RegexpLikePredicate(ExpressionContext lhs, String value) {
super(lhs);
@@ -55,11 +66,17 @@ public class RegexpLikePredicate extends BasePredicate {
return _caseInsensitive;
}
+ /// Returns the compiled pattern, lazily compiling and caching it on first
access.
+ ///
+ /// Uses the racy-single-check idiom: two threads may each compile the
pattern, but both compile the same one, so
+ /// the duplicate work is harmless. Correctness relies on `_pattern` being
`volatile`; see the field for why.
public Pattern getPattern() {
- if (_pattern == null) {
- _pattern = PatternFactory.compile(_value, _caseInsensitive);
+ Pattern pattern = _pattern;
+ if (pattern == null) {
+ pattern = PatternFactory.compile(_value, _caseInsensitive);
+ _pattern = pattern;
}
- return _pattern;
+ return pattern;
}
@Override
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/utils/regex/JavaUtilPattern.java
b/pinot-common/src/main/java/org/apache/pinot/common/utils/regex/JavaUtilPattern.java
index 88d61647240..48d68b6512a 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/utils/regex/JavaUtilPattern.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/utils/regex/JavaUtilPattern.java
@@ -21,7 +21,7 @@ package org.apache.pinot.common.utils.regex;
import java.util.regex.Pattern;
public class JavaUtilPattern implements
org.apache.pinot.common.utils.regex.Pattern {
- final Pattern _pattern;
+ private final Pattern _pattern;
public JavaUtilPattern(String regex) {
_pattern = Pattern.compile(regex, 0);
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/utils/regex/Re2jPattern.java
b/pinot-common/src/main/java/org/apache/pinot/common/utils/regex/Re2jPattern.java
index 24e7e819e04..2f52d9131ae 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/utils/regex/Re2jPattern.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/utils/regex/Re2jPattern.java
@@ -22,7 +22,7 @@ import com.google.re2j.Pattern;
public class Re2jPattern implements
org.apache.pinot.common.utils.regex.Pattern {
- Pattern _pattern;
+ private final Pattern _pattern;
public Re2jPattern(String regex) {
this(regex, false);
diff --git
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/AbstractIndexType.java
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/AbstractIndexType.java
index ced61841acf..09ee848c360 100644
---
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/AbstractIndexType.java
+++
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/AbstractIndexType.java
@@ -39,8 +39,15 @@ public abstract class AbstractIndexType<C extends
IndexConfig, IR extends IndexR
implements IndexType<C, IR, IC> {
private final String _id;
- private ColumnConfigDeserializer<C> _deserializer;
- private IndexReaderFactory<IR> _readerFactory;
+
+ /// Lazily created caches of [#getConfig] and [#getReaderFactory].
+ ///
+ /// `volatile` is required, not just for the null checks in those methods:
an index type is a process-wide singleton
+ /// held by [IndexService] and is used concurrently by the threads that
load, reload and refresh segments. Without
+ /// `volatile` these values are published unsafely, so a racing thread can
read the non-null reference while the
+ /// contents are not yet visible to it.
+ private volatile ColumnConfigDeserializer<C> _deserializer;
+ private volatile IndexReaderFactory<IR> _readerFactory;
protected ColumnConfigDeserializer<C> createDeserializer() {
ColumnConfigDeserializer<C> fromIndexes =
@@ -70,22 +77,30 @@ public abstract class AbstractIndexType<C extends
IndexConfig, IR extends IndexR
@Override
public Map<String, C> getConfig(TableConfig tableConfig, Schema schema) {
- if (_deserializer == null) {
- _deserializer = createDeserializer();
+ ColumnConfigDeserializer<C> deserializer = _deserializer;
+ if (deserializer == null) {
+ deserializer = createDeserializer();
+ _deserializer = deserializer;
}
try {
- return _deserializer.deserialize(tableConfig, schema);
+ return deserializer.deserialize(tableConfig, schema);
} catch (MergedColumnConfigDeserializer.ConfigDeclaredTwiceException ex) {
throw new
MergedColumnConfigDeserializer.ConfigDeclaredTwiceException(ex.getColumn(),
this, ex);
}
}
+ /// Returns the reader factory, lazily creating and caching it on first
access.
+ ///
+ /// Uses the racy-single-check idiom: two threads may each create a factory,
but the factories are equivalent, so
+ /// the duplicate work is harmless. Correctness relies on `_readerFactory`
being `volatile`; see the field for why.
@Override
public IndexReaderFactory<IR> getReaderFactory() {
- if (_readerFactory == null) {
- _readerFactory = createReaderFactory();
+ IndexReaderFactory<IR> readerFactory = _readerFactory;
+ if (readerFactory == null) {
+ readerFactory = createReaderFactory();
+ _readerFactory = readerFactory;
}
- return _readerFactory;
+ return readerFactory;
}
public void convertToNewFormat(TableConfig tableConfig, Schema schema) {
diff --git
a/pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFieldSpec.java
b/pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFieldSpec.java
index 959aecbda09..e284600ac0f 100644
--- a/pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFieldSpec.java
+++ b/pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeFieldSpec.java
@@ -32,8 +32,13 @@ public final class DateTimeFieldSpec extends FieldSpec {
private String _format;
private String _granularity;
private Object _sampleValue;
- private transient DateTimeFormatSpec _formatSpec;
- private transient DateTimeGranularitySpec _granularitySpec;
+ /// Lazily derived caches of [#getFormatSpec] and [#getGranularitySpec].
+ ///
+ /// `volatile` is required, not just for the null checks in those getters: a
field spec belongs to a `Schema`, which
+ /// is cached and read concurrently by query threads. Without it these
values are published unsafely, so a racing
+ /// thread can read the non-null reference while the contents are not yet
visible to it.
+ private transient volatile DateTimeFormatSpec _formatSpec;
+ private transient volatile DateTimeGranularitySpec _granularitySpec;
public enum TimeFormat {
EPOCH, TIMESTAMP, SIMPLE_DATE_FORMAT
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
