gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136771948
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/BloomFilterConfig.java:
##########
@@ -21,20 +21,27 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Preconditions;
-import org.apache.pinot.spi.config.BaseJsonConfig;
+import java.util.Objects;
-public class BloomFilterConfig extends BaseJsonConfig {
+public class BloomFilterConfig extends IndexConfig {
public static final double DEFAULT_FPP = 0.05;
+ private static final BloomFilterConfig DEFAULT = new
BloomFilterConfig(BloomFilterConfig.DEFAULT_FPP, 0, false);
+ public static final BloomFilterConfig DISABLED = new
BloomFilterConfig(false, BloomFilterConfig.DEFAULT_FPP, 0,
Review Comment:
Should they? The first implementation used a monadic structure where
disabled indexes were maker by a wrapper called `IndexImplementation<? extends
IndexConfig>`. In that implementation `IndexConfig` instances where always
"correct" in the sense that for example `BloomFilterConfig.getFpp()` always
returned the actual value that should be used. Otherwise, thanks to the
properties of `IndexImplementation`, you would get a compile exception.
@Jackie-Jiang and I were discussing about that and decided to simplify the
architecture by introducing the disabled/enabled concept in `IndexConfig`
itself. Now in order to know whether an attribute (like `getFpp`) contains a
valid value or not we always need to verify whether `isEnabled()` is true or
not.
Also, in some properties is quite difficult to define an _invalid_ value. If
we are measuring a length, a negative value can be an _invalid_ value, but what
would be an invalid value for a property that is
[surjective](https://en.wikipedia.org/wiki/Surjective_function) (meaning that
all values are valid). For example, if the property is a name or a seed.
You will see in following PRs that in some configs I indistinctly used
either java default values (0 for numbers, etc), domain default values (like ""
for strings) or actual invalid values (like -1 for lengths). But actually that
is not important, as the caller _has_ to verify whether the config `isEnabled`
or not before using it.
We could also add checks on each get method that verifies whether the config
is enabled or not and throw `IllegalStateException` in case they don't. TBH I
think that is a decision that should be taken by whoever creates the index and
is something not very important for the `index-spi` feature.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
