Jackie-Jiang commented on code in PR #18317:
URL: https://github.com/apache/pinot/pull/18317#discussion_r3268885041
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/filter/ExactVectorScanFilterOperatorTest.java:
##########
@@ -238,7 +238,7 @@ private ForwardIndexReader<?>
createMockForwardIndexReader(float[][] vectors) {
private VectorIndexConfig createVectorIndexConfig(String backendType,
VectorIndexConfig.VectorDistanceFunction distanceFunction) {
- return new VectorIndexConfig(false, backendType, 2, 1, distanceFunction,
+ return new VectorIndexConfig(Boolean.FALSE, backendType, 2, 1,
distanceFunction,
Review Comment:
These changes are not necessary. Same for other changes in test/benchmark
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/BloomFilterConfig.java:
##########
@@ -20,49 +20,78 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonValue;
+import com.fasterxml.jackson.databind.node.ObjectNode;
import com.google.common.base.Preconditions;
import java.util.Objects;
public class BloomFilterConfig extends IndexConfig {
public static final double DEFAULT_FPP = 0.05;
- public static final BloomFilterConfig DEFAULT = new
BloomFilterConfig(BloomFilterConfig.DEFAULT_FPP, 0, false);
- public static final BloomFilterConfig DISABLED = new BloomFilterConfig(true,
BloomFilterConfig.DEFAULT_FPP, 0,
- false);
+ public static final BloomFilterConfig DEFAULT = new BloomFilterConfig(null,
null, null, null);
+ public static final BloomFilterConfig DISABLED = new BloomFilterConfig(true,
null, null, null);
- private final double _fpp;
- private final int _maxSizeInBytes;
- private final boolean _loadOnHeap;
+ private final Double _fpp;
+ private final Integer _maxSizeInBytes;
+ private final Boolean _loadOnHeap;
public BloomFilterConfig(double fpp, int maxSizeInBytes, boolean loadOnHeap)
{
- this(false, fpp, maxSizeInBytes, loadOnHeap);
+ // Forward to the wrapper @JsonCreator. Boolean.FALSE for arg0
disambiguates from the deprecated
+ // primitive overload below (which would otherwise tie on overload
resolution).
+ this(Boolean.FALSE, Double.valueOf(fpp), Integer.valueOf(maxSizeInBytes),
Boolean.valueOf(loadOnHeap));
+ }
+
+ /// Binary-compat delegate for the pre-wrapper `(Boolean, double, int,
boolean)` ctor. Kept so existing
+ /// compiled call sites resolved to the primitive signature continue to
link. Prefer the wrapper-typed
+ /// `@JsonCreator` ctor below for new code.
+ @Deprecated
+ public BloomFilterConfig(Boolean disabled, double fpp, int maxSizeInBytes,
boolean loadOnHeap) {
+ this(disabled, Double.valueOf(fpp), Integer.valueOf(maxSizeInBytes),
Boolean.valueOf(loadOnHeap));
}
@JsonCreator
- public BloomFilterConfig(@JsonProperty("disabled") Boolean disabled,
@JsonProperty(value = "fpp") double fpp,
- @JsonProperty(value = "maxSizeInBytes") int maxSizeInBytes,
- @JsonProperty(value = "loadOnHeap") boolean loadOnHeap) {
+ public BloomFilterConfig(@JsonProperty("disabled") Boolean disabled,
@JsonProperty(value = "fpp") Double fpp,
+ @JsonProperty(value = "maxSizeInBytes") Integer maxSizeInBytes,
+ @JsonProperty(value = "loadOnHeap") Boolean loadOnHeap) {
super(disabled);
- if (fpp != 0.0) {
+ if (fpp != null && fpp != 0.0) {
Preconditions.checkArgument(fpp > 0.0 && fpp < 1.0, "Invalid fpp (false
positive probability): %s", fpp);
- _fpp = fpp;
- } else {
- _fpp = DEFAULT_FPP;
}
+ _fpp = fpp;
_maxSizeInBytes = maxSizeInBytes;
_loadOnHeap = loadOnHeap;
}
public double getFpp() {
- return _fpp;
+ return _fpp != null && _fpp != 0.0 ? _fpp : DEFAULT_FPP;
}
public int getMaxSizeInBytes() {
- return _maxSizeInBytes;
+ return _maxSizeInBytes != null ? _maxSizeInBytes : 0;
}
public boolean isLoadOnHeap() {
- return _loadOnHeap;
+ return Boolean.TRUE.equals(_loadOnHeap);
+ }
+
+ /// Curated slim serializer. See [IndexConfig#toJsonObject()] for the
rationale.
+ ///
+ /// Each field is emitted only when it was explicitly configured (non-null).
Sentinel `fpp == 0.0` is also
+ /// emitted as-is — the runtime getter still resolves to [#DEFAULT_FPP] for
that legacy sentinel.
+ @Override
+ @JsonValue
Review Comment:
(nit) Do we need `@JsonValue` override in all places, or just the base one
is good enough?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/IndexConfig.java:
##########
@@ -21,37 +21,53 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonValue;
+import com.fasterxml.jackson.databind.node.ObjectNode;
import java.util.Objects;
import org.apache.pinot.spi.config.BaseJsonConfig;
+import org.apache.pinot.spi.utils.JsonUtils;
-/**
- * This is the base class used to configure indexes.
- *
- * The common logic between all indexes is that they can be enabled or
disabled.
- *
- * Indexes that do not require extra configuration can directly use this class.
- */
+/// Base class for index configs.
+///
+/// Common logic across all indexes: each can be enabled or disabled. Indexes
with no extra
+/// configuration can use this class directly.
public class IndexConfig extends BaseJsonConfig {
- public static final IndexConfig ENABLED = new IndexConfig(false);
+ public static final IndexConfig ENABLED = new IndexConfig((Boolean) null);
public static final IndexConfig DISABLED = new IndexConfig(true);
- private final boolean _disabled;
+ private final Boolean _disabled;
- /**
- * @param disabled whether the config is disabled. Null is considered
enabled.
- */
+ /// @param disabled whether the config is disabled. `null` and `false` both
mean enabled — explicit `false` is
+ /// normalized to `null` so the slim form omits the key (every index
defaults to enabled and that default is
+ /// extremely unlikely to flip).
@JsonCreator
public IndexConfig(@JsonProperty("disabled") Boolean disabled) {
- _disabled = Boolean.TRUE.equals(disabled);
+ _disabled = Boolean.TRUE.equals(disabled) ? Boolean.TRUE : null;
}
public boolean isDisabled() {
- return _disabled;
+ return Boolean.TRUE.equals(_disabled);
}
@JsonIgnore
public boolean isEnabled() {
- return !_disabled;
+ return !isDisabled();
+ }
+
+ /// Curated Jackson serializer. Annotated with [JsonValue] so Jackson skips
default bean introspection and uses
+ /// this method as the sole source of truth, emitting only fields that were
explicitly configured (non-null).
+ /// Subclasses override and call `super.toJsonObject()` first to inherit the
`disabled` key handling.
+ ///
+ /// This mirrors the slim-serialization pattern introduced for `Schema` and
`TableConfigs` in apache/pinot#17558
+ /// and ensures user-supplied slim index configs do not get fattened with
defaults on the first round-trip
+ /// through any Pinot `ObjectMapper`.
+ @JsonValue
+ public ObjectNode toJsonObject() {
Review Comment:
We can mark `BaseJsonConfig.toJsonNode()` as `@JsonValue`. They serve the
same purpose.
We can also remove `toJsonObject()` in `TableConfigs` and override this
method instead. It makes things easier to track
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/IndexConfig.java:
##########
@@ -21,37 +21,53 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonValue;
+import com.fasterxml.jackson.databind.node.ObjectNode;
import java.util.Objects;
import org.apache.pinot.spi.config.BaseJsonConfig;
+import org.apache.pinot.spi.utils.JsonUtils;
-/**
- * This is the base class used to configure indexes.
- *
- * The common logic between all indexes is that they can be enabled or
disabled.
- *
- * Indexes that do not require extra configuration can directly use this class.
- */
+/// Base class for index configs.
+///
+/// Common logic across all indexes: each can be enabled or disabled. Indexes
with no extra
+/// configuration can use this class directly.
public class IndexConfig extends BaseJsonConfig {
- public static final IndexConfig ENABLED = new IndexConfig(false);
+ public static final IndexConfig ENABLED = new IndexConfig((Boolean) null);
public static final IndexConfig DISABLED = new IndexConfig(true);
- private final boolean _disabled;
+ private final Boolean _disabled;
Review Comment:
I think we can keep `_disabled` as primitive, and only include it when it is
`true`. It is a private variable, so shouldn't cause any issue.
We can consider changing the constructor to take primitive `boolean
disabled`. I don't think we will ever allow changing default for this field
--
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]
