You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2023/01/27 14:48:11 UTC
[GitHub] [pinot] gortiz opened a new pull request, #10192: [draft] Index spi: index service
gortiz opened a new pull request, #10192:
URL: https://github.com/apache/pinot/pull/10192
This is a partial draft PR related to https://github.com/apache/pinot/issues/10183. The whole PR is in https://github.com/apache/pinot/pull/10184. It isn't intended to be merged as it is and it doesn't require to compile or pass the checks. It has been created in order to make it easier to understand the PR with all the changes.
This PR depends on https://github.com/apache/pinot/pull/10191. Readers only interested in the difference between this and the previous PR can focus on https://github.com/gortiz/pinot/pull/1
This PR shows the basics of the proposed IndexService without actually modifying the code and also includes a BloomIndexPlugin as an example. Here we use Java Service Provider Interface to discover all services that implement IndexPlugin, although the procedure to discover the instances is subject of discussion.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10192: [draft] Index spi: index service
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10192:
URL: https://github.com/apache/pinot/pull/10192#issuecomment-1408201675
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10192](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8e8edc1) into [master](https://codecov.io/gh/apache/pinot/commit/0797e05da61c37629da3e99bdeb4d96dc60d7911?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0797e05) will **decrease** coverage by `44.40%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10192 +/- ##
=============================================
- Coverage 69.02% 24.62% -44.40%
+ Complexity 5800 48 -5752
=============================================
Files 2020 2014 -6
Lines 109138 108731 -407
Branches 16570 16526 -44
=============================================
- Hits 75328 26774 -48554
- Misses 28508 79146 +50638
+ Partials 5302 2811 -2491
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.62% <0.00%> (?)` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...nt/local/segment/index/bloom/BloomIndexPlugin.java](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2Jsb29tL0Jsb29tSW5kZXhQbHVnaW4uamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ment/local/segment/index/bloom/BloomIndexType.java](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2Jsb29tL0Jsb29tSW5kZXhUeXBlLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...che/pinot/segment/spi/index/FieldIndexConfigs.java](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L0ZpZWxkSW5kZXhDb25maWdzLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...g/apache/pinot/segment/spi/index/IndexCreator.java](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L0luZGV4Q3JlYXRvci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/segment/spi/index/IndexDeclaration.java](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L0luZGV4RGVjbGFyYXRpb24uamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ment/spi/index/IndexReaderConstraintException.java](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L0luZGV4UmVhZGVyQ29uc3RyYWludEV4Y2VwdGlvbi5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...g/apache/pinot/segment/spi/index/IndexService.java](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L0luZGV4U2VydmljZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| [.../org/apache/pinot/segment/spi/index/IndexType.java](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L0luZGV4VHlwZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...pache/pinot/segment/spi/index/StandardIndexes.java](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L1N0YW5kYXJkSW5kZXhlcy5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1553 more](https://codecov.io/gh/apache/pinot/pull/10192?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1138564068
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/bloom/BloomIndexType.java:
##########
@@ -0,0 +1,109 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.local.segment.index.bloom;
+
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.segment.creator.impl.bloom.OnHeapGuavaBloomFilterCreator;
+import org.apache.pinot.segment.local.segment.index.readers.bloom.BloomFilterReaderFactory;
+import org.apache.pinot.segment.spi.ColumnMetadata;
+import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.V1Constants;
+import org.apache.pinot.segment.spi.creator.IndexCreationContext;
+import org.apache.pinot.segment.spi.index.FieldIndexConfigs;
+import org.apache.pinot.segment.spi.index.IndexHandler;
+import org.apache.pinot.segment.spi.index.IndexReaderFactory;
+import org.apache.pinot.segment.spi.index.IndexType;
+import org.apache.pinot.segment.spi.index.creator.BloomFilterCreator;
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.segment.spi.memory.PinotDataBuffer;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.Schema;
+
+public class BloomIndexType implements IndexType<BloomFilterConfig, BloomFilterReader, BloomFilterCreator> {
+ public static final BloomIndexType INSTANCE = new BloomIndexType();
+
+ @Override
+ public String getId() {
+ return "bloom_filter";
+ }
+
+ @Override
+ public Class<BloomFilterConfig> getIndexConfigClass() {
+ return BloomFilterConfig.class;
+ }
+
+ @Override
+ public BloomFilterConfig getDefaultConfig() {
+ return BloomFilterConfig.DISABLED;
+ }
+
+ @Override
+ public BloomFilterConfig getConfig(TableConfig tableConfig, Schema schema) {
+ throw new UnsupportedOperationException("To be implemented in a future PR");
+ }
+
+ @Override
+ public BloomFilterCreator createIndexCreator(IndexCreationContext context, BloomFilterConfig indexConfig) {
+ int cardinality = context.getCardinality();
+ if (cardinality == Constants.UNKNOWN_CARDINALITY) {
+ // This is when we're creating bloom filters for non dictionary encoded cols where exact cardinality is not
+ // known beforehand.
+ // Since this field is only used for the estimate cardinality, using total # of entries instead
+ // TODO (saurabh) Check if we can do a better estimate
+ cardinality = context.getTotalNumberOfEntries();
+ }
+ return new OnHeapGuavaBloomFilterCreator(context.getIndexDir(), context.getFieldSpec().getName(), cardinality,
+ indexConfig, context.getFieldSpec().getDataType());
+ }
+
+ @Override
+ public IndexReaderFactory<BloomFilterReader> getReaderFactory() {
+ return new IndexReaderFactory.Default<BloomFilterConfig, BloomFilterReader>() {
Review Comment:
Not just the GC, I referred to [scalar replacement](https://shipilev.net/jvm/anatomy-quarks/18-scalar-replacement/), but I'll change it given that seems clear that it worries several commiters
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/bloom/BloomIndexType.java:
##########
@@ -0,0 +1,109 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.local.segment.index.bloom;
+
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.segment.creator.impl.bloom.OnHeapGuavaBloomFilterCreator;
+import org.apache.pinot.segment.local.segment.index.readers.bloom.BloomFilterReaderFactory;
+import org.apache.pinot.segment.spi.ColumnMetadata;
+import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.V1Constants;
+import org.apache.pinot.segment.spi.creator.IndexCreationContext;
+import org.apache.pinot.segment.spi.index.FieldIndexConfigs;
+import org.apache.pinot.segment.spi.index.IndexHandler;
+import org.apache.pinot.segment.spi.index.IndexReaderFactory;
+import org.apache.pinot.segment.spi.index.IndexType;
+import org.apache.pinot.segment.spi.index.creator.BloomFilterCreator;
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.segment.spi.memory.PinotDataBuffer;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.Schema;
+
+public class BloomIndexType implements IndexType<BloomFilterConfig, BloomFilterReader, BloomFilterCreator> {
+ public static final BloomIndexType INSTANCE = new BloomIndexType();
+
+ @Override
+ public String getId() {
+ return "bloom_filter";
+ }
+
+ @Override
+ public Class<BloomFilterConfig> getIndexConfigClass() {
+ return BloomFilterConfig.class;
+ }
+
+ @Override
+ public BloomFilterConfig getDefaultConfig() {
+ return BloomFilterConfig.DISABLED;
+ }
+
+ @Override
+ public BloomFilterConfig getConfig(TableConfig tableConfig, Schema schema) {
+ throw new UnsupportedOperationException("To be implemented in a future PR");
+ }
+
+ @Override
+ public BloomFilterCreator createIndexCreator(IndexCreationContext context, BloomFilterConfig indexConfig) {
+ int cardinality = context.getCardinality();
+ if (cardinality == Constants.UNKNOWN_CARDINALITY) {
+ // This is when we're creating bloom filters for non dictionary encoded cols where exact cardinality is not
+ // known beforehand.
+ // Since this field is only used for the estimate cardinality, using total # of entries instead
+ // TODO (saurabh) Check if we can do a better estimate
+ cardinality = context.getTotalNumberOfEntries();
+ }
+ return new OnHeapGuavaBloomFilterCreator(context.getIndexDir(), context.getFieldSpec().getName(), cardinality,
+ indexConfig, context.getFieldSpec().getDataType());
+ }
+
+ @Override
+ public IndexReaderFactory<BloomFilterReader> getReaderFactory() {
+ return new IndexReaderFactory.Default<BloomFilterConfig, BloomFilterReader>() {
Review Comment:
Not just the GC, I referred to [scalar replacement](https://shipilev.net/jvm/anatomy-quarks/18-scalar-replacement/), but I'll change it given that seems clear that it worries several committers
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136796477
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
+
+ private static volatile IndexService _instance = fromServiceLoader();
+
+ private final Set<IndexType<?, ?, ?>> _allIndexes;
+
+ public IndexService(Set<IndexPlugin<?>> allPlugins) {
+ _allIndexes = Sets.newHashSetWithExpectedSize(allPlugins.size());
+
+ for (IndexPlugin<?> plugin : allPlugins) {
+ _allIndexes.add(plugin.getIndexType());
+ }
+ }
+
+ public static IndexService getInstance() {
+ return _instance;
+ }
+
+ public static void setInstance(IndexService other) {
+ _instance = other;
+ }
+
+ public static IndexService fromServiceLoader() {
+ Set<IndexPlugin<?>> pluginList = new HashSet<>();
+ for (IndexPlugin indexPlugin : ServiceLoader.load(IndexPlugin.class)) {
+ pluginList.add(indexPlugin);
+ }
+ return new IndexService(pluginList);
+ }
+
+ public Set<IndexType<?, ?, ?>> getAllIndexes() {
+ return _allIndexes;
+ }
+
+ public Optional<IndexType<?, ?, ?>> getIndexTypeById(String indexId) {
+ return getAllIndexes().stream().filter(indexType -> indexType.getId().equalsIgnoreCase(indexId)).findAny();
+ }
+
+ public IndexType<?, ?, ?> getIndexTypeByIdOrThrow(String indexId) {
+ return getIndexTypeById(indexId)
+ .orElseThrow(() -> new IllegalArgumentException("Unknown index id: " + indexId));
+ }
Review Comment:
First of all, these methods were renamed in `index-spi-all-types` branch. I think it was a mistake to do not change them here, so I'm going to apply the changes. The new names are `get(String indexId)` and `getOrThrow(String indexId)`.
> What are the allowed values for the indexId string? Can you add javadocs to these methods?
I thought it was clear that `indexId` is the result of calling `IndexType.getId()`, but I can add extra javadoc in order to make it clear.
What I'm sure it may not be clear is where we need it. It is going to be easier to understand later when these methods are actually used. But I can also add some reasons in the javadoc.
> Do we really need both methods - one that throws and the one which doesn't?
First of all, I don't think it actually hurts to have both methods. The main reason to have these methods is to be able to deal with the case where caller ask for an index that doesn't exist without producing NPE.
If we would only had a single method `get`, it should either:
1. Return a nullable `IndexType`.
2. Throw when there is no index with the given id.
3. Return `Optional<IndexType>`.
Option 1 makes it very easy to the caller to forget that the index may not be loaded, which would produce a NPE the first time it tries to use it, but probably not in the same line where `get` is called, so it is more difficult to find where the problem is.
Option 2 is very aggressive. Either we add another method to check whether the index is there or the caller would need to write something like the following in order to deal with the fact that an index may not be loaded
```java
boolean indexIsLoaded;
try {
IndexService.getInstance().get()
indexIsLoaded = true;
} catch (Whatever e) {
indexIsLoaded = false;
}
```
Option 3 is safe at compile time and is the one I'm following here. There are two problems with this approach:
1. An Optional is created each time the method is called with an index that actually exists. This shouldn't be a performance issue, but given that index types do not actually change in production, we can catch the optionals in case we found that we are allocating too much here.
2. Most of the times we call this method, the index type is a precondition of the calling code, therefore the caller actually want to fail if the index type is not there. Therefore we needed to repeat the patter `IndexService.getInstance().get().orElseThrow()` several times.
That is why I added the second method and if you see https://github.com/apache/pinot/pull/10184 you will see that most (if not all) calls are to `getOrThrow` (directly or indirectly by using `StandardIndexes`).
What we can do, if you think that it is interesting, is to change the semantics of the methods. `get` could throw if the index type is not there and a new method `getOptional` or `getNullable` may return Optional or null in case the index is not present.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #10192: Index spi: index service
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136562472
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
Review Comment:
nit: @ThreadSafe annotation
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
+
+ private static volatile IndexService _instance = fromServiceLoader();
+
+ private final Set<IndexType<?, ?, ?>> _allIndexes;
+
+ public IndexService(Set<IndexPlugin<?>> allPlugins) {
+ _allIndexes = Sets.newHashSetWithExpectedSize(allPlugins.size());
+
+ for (IndexPlugin<?> plugin : allPlugins) {
+ _allIndexes.add(plugin.getIndexType());
+ }
+ }
+
+ public static IndexService getInstance() {
+ return _instance;
+ }
+
+ public static void setInstance(IndexService other) {
+ _instance = other;
+ }
+
+ public static IndexService fromServiceLoader() {
+ Set<IndexPlugin<?>> pluginList = new HashSet<>();
+ for (IndexPlugin indexPlugin : ServiceLoader.load(IndexPlugin.class)) {
+ pluginList.add(indexPlugin);
+ }
+ return new IndexService(pluginList);
+ }
+
+ public Set<IndexType<?, ?, ?>> getAllIndexes() {
+ return _allIndexes;
+ }
+
+ public Optional<IndexType<?, ?, ?>> getIndexTypeById(String indexId) {
+ return getAllIndexes().stream().filter(indexType -> indexType.getId().equalsIgnoreCase(indexId)).findAny();
Review Comment:
Could this indexId be an enum instead?
##########
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);
Review Comment:
public and remove createDefault()?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/StandardIndexes.java:
##########
@@ -0,0 +1,39 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+
+
+@SuppressWarnings("unchecked")
+public class StandardIndexes {
Review Comment:
What's the use of this class?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136754782
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/bloom/BloomIndexType.java:
##########
@@ -0,0 +1,109 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.local.segment.index.bloom;
+
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.segment.creator.impl.bloom.OnHeapGuavaBloomFilterCreator;
+import org.apache.pinot.segment.local.segment.index.readers.bloom.BloomFilterReaderFactory;
+import org.apache.pinot.segment.spi.ColumnMetadata;
+import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.V1Constants;
+import org.apache.pinot.segment.spi.creator.IndexCreationContext;
+import org.apache.pinot.segment.spi.index.FieldIndexConfigs;
+import org.apache.pinot.segment.spi.index.IndexHandler;
+import org.apache.pinot.segment.spi.index.IndexReaderFactory;
+import org.apache.pinot.segment.spi.index.IndexType;
+import org.apache.pinot.segment.spi.index.creator.BloomFilterCreator;
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.segment.spi.memory.PinotDataBuffer;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.Schema;
+
+public class BloomIndexType implements IndexType<BloomFilterConfig, BloomFilterReader, BloomFilterCreator> {
+ public static final BloomIndexType INSTANCE = new BloomIndexType();
+
+ @Override
+ public String getId() {
+ return "bloom_filter";
+ }
+
+ @Override
+ public Class<BloomFilterConfig> getIndexConfigClass() {
+ return BloomFilterConfig.class;
+ }
+
+ @Override
+ public BloomFilterConfig getDefaultConfig() {
+ return BloomFilterConfig.DISABLED;
+ }
+
+ @Override
+ public BloomFilterConfig getConfig(TableConfig tableConfig, Schema schema) {
+ throw new UnsupportedOperationException("To be implemented in a future PR");
+ }
+
+ @Override
+ public BloomFilterCreator createIndexCreator(IndexCreationContext context, BloomFilterConfig indexConfig) {
+ int cardinality = context.getCardinality();
+ if (cardinality == Constants.UNKNOWN_CARDINALITY) {
+ // This is when we're creating bloom filters for non dictionary encoded cols where exact cardinality is not
+ // known beforehand.
+ // Since this field is only used for the estimate cardinality, using total # of entries instead
+ // TODO (saurabh) Check if we can do a better estimate
+ cardinality = context.getTotalNumberOfEntries();
+ }
+ return new OnHeapGuavaBloomFilterCreator(context.getIndexDir(), context.getFieldSpec().getName(), cardinality,
+ indexConfig, context.getFieldSpec().getDataType());
+ }
+
+ @Override
+ public IndexReaderFactory<BloomFilterReader> getReaderFactory() {
+ return new IndexReaderFactory.Default<BloomFilterConfig, BloomFilterReader>() {
+ @Override
+ protected IndexType<BloomFilterConfig, BloomFilterReader, ?> getIndexType() {
+ return BloomIndexType.INSTANCE;
+ }
+
+ @Override
+ protected BloomFilterReader createIndexReader(PinotDataBuffer dataBuffer, ColumnMetadata metadata,
+ BloomFilterConfig indexConfig) {
+ return BloomFilterReaderFactory.getBloomFilterReader(dataBuffer, indexConfig.isLoadOnHeap());
Review Comment:
Yes, we can do it. I just wanted to minimize the number of changes in order to make the PR easier to read, but we can modify that in `index-spi` branch. Please note that this PR includes `BloomIndexType` as just an example and the implementation doesn't actually matter that much, the next PR (https://github.com/apache/pinot/pull/10193) is where other indexes are created and later in https://github.com/apache/pinot/pull/10184 is where most of the changes actually may (or may not :D) make sense.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136848647
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
Review Comment:
> In general, it is simpler to maintain immutable instances of objects
Totally agree. I try to make objects immutable unless there is a strong reason to not to. In this case is even worse. We are not making the instance mutable. We are making the static state mutable, which usually is a red flag. I really tried to avoid this, but right now this is the only way to do this that I found.
So there are two different questions here:
1. One is why would we need to have a IndexService that is not created from the classpath.
2. Assuming we need an IndexService that is not created from the classpath, how could we have that feature without modifying static state.
I think this [other discussion](https://github.com/apache/pinot/pull/10192/files/42a5f0ed55636f5d9acfc9125b4596ff40ce670e#r1136583129) is also related to the first question, so I'm going to answer that there and I'm going to focus here on the second question.
About the (ab)use of static state in Pinot: I think that is a flaw in the current Pinot architecture. Unless I'm wrong, we don't have other ways to have a shared state in different parts of the code unless we use static variables.
TL;DR: I think we need to add some contextual object (and probably some dependency injection) into Pinot in order to be able to make it more complex without abusing of static state.
Long story:
IMHO Pinot abuse of static state (here, in metrics, now in PlanMaker, etc). A mentor I had told me once that one of the most important things he focus on when reviewing an architectural change in the code is whether several instance of the code can be running in the same process. He wasn't talking about Java instances but _service_ instances. In Pinot case, it would be whether two Pinot servers or two Pinot brokers can run in the same process.
This idea looks quite artificial. What is the business advantage of running two servers in the same process? Probably none! You would probably would never want to do that in production! But this idea produces better architectures. For example, if you can run two servers in the same process, testing is quite easier. Also, suddenly you cannot use static variables to store information that will change from one server to the other. If you cannot use static variables to do that, you need to have some kind of Context class that you can inject into your instances in order to get the information related to each server that is running in the same process. And forcing you to use this Context object suddenly makes a lot of problems easier to solve:
* How can each server in the same process view different indexes? It is simple, just store an `IndexService` instance on the Context instead of a static variable!
* How can each server has their own metrics? It is simple, just store the MetricRegistry in the Context instead of using an static variable! And each meter should have a prefix that identifies the specific server.
* How can I change the way my PlanMaker changes a leaf node depending on whether an index is or is not in the IndexService? Again, add that to the Context.
This Context object doesn't exist in Pinot because it wasn't designed with this architectural idea in mind. And Pinot is a great product without it! But the lack of this concept makes more difficult to test Pinot or to create complex behaviors. For example, to test the behavior of your code using different index type sets, you need to change a static variable, which means that running tests in parallel is quite difficult.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136993116
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/StandardIndexes.java:
##########
@@ -0,0 +1,39 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+
+
+@SuppressWarnings("unchecked")
+public class StandardIndexes {
Review Comment:
Added. The actual usage is in https://github.com/apache/pinot/pull/10194
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #10192: Index spi: index service
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1137848288
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
+
+ private static volatile IndexService _instance = fromServiceLoader();
+
+ private final Set<IndexType<?, ?, ?>> _allIndexes;
+
+ public IndexService(Set<IndexPlugin<?>> allPlugins) {
Review Comment:
I feel the ctor should still be private. To solve the problem for test and production customization, the factory method should probably take some configuration.
Otherwise, some code outside index service needs to handle the customized logic. If we take a configuration, by default, it loads from service loader. Otherwise, it builds based on configuration.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] saurabhd336 commented on a diff in pull request #10192: Index spi: index service
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136655780
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/bloom/BloomIndexType.java:
##########
@@ -0,0 +1,109 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.local.segment.index.bloom;
+
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.segment.creator.impl.bloom.OnHeapGuavaBloomFilterCreator;
+import org.apache.pinot.segment.local.segment.index.readers.bloom.BloomFilterReaderFactory;
+import org.apache.pinot.segment.spi.ColumnMetadata;
+import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.V1Constants;
+import org.apache.pinot.segment.spi.creator.IndexCreationContext;
+import org.apache.pinot.segment.spi.index.FieldIndexConfigs;
+import org.apache.pinot.segment.spi.index.IndexHandler;
+import org.apache.pinot.segment.spi.index.IndexReaderFactory;
+import org.apache.pinot.segment.spi.index.IndexType;
+import org.apache.pinot.segment.spi.index.creator.BloomFilterCreator;
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.segment.spi.memory.PinotDataBuffer;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.Schema;
+
+public class BloomIndexType implements IndexType<BloomFilterConfig, BloomFilterReader, BloomFilterCreator> {
+ public static final BloomIndexType INSTANCE = new BloomIndexType();
+
+ @Override
+ public String getId() {
+ return "bloom_filter";
+ }
+
+ @Override
+ public Class<BloomFilterConfig> getIndexConfigClass() {
+ return BloomFilterConfig.class;
+ }
+
+ @Override
+ public BloomFilterConfig getDefaultConfig() {
+ return BloomFilterConfig.DISABLED;
+ }
+
+ @Override
+ public BloomFilterConfig getConfig(TableConfig tableConfig, Schema schema) {
+ throw new UnsupportedOperationException("To be implemented in a future PR");
+ }
+
+ @Override
+ public BloomFilterCreator createIndexCreator(IndexCreationContext context, BloomFilterConfig indexConfig) {
+ int cardinality = context.getCardinality();
+ if (cardinality == Constants.UNKNOWN_CARDINALITY) {
+ // This is when we're creating bloom filters for non dictionary encoded cols where exact cardinality is not
+ // known beforehand.
+ // Since this field is only used for the estimate cardinality, using total # of entries instead
+ // TODO (saurabh) Check if we can do a better estimate
+ cardinality = context.getTotalNumberOfEntries();
+ }
+ return new OnHeapGuavaBloomFilterCreator(context.getIndexDir(), context.getFieldSpec().getName(), cardinality,
+ indexConfig, context.getFieldSpec().getDataType());
+ }
+
+ @Override
+ public IndexReaderFactory<BloomFilterReader> getReaderFactory() {
+ return new IndexReaderFactory.Default<BloomFilterConfig, BloomFilterReader>() {
+ @Override
+ protected IndexType<BloomFilterConfig, BloomFilterReader, ?> getIndexType() {
+ return BloomIndexType.INSTANCE;
+ }
+
+ @Override
+ protected BloomFilterReader createIndexReader(PinotDataBuffer dataBuffer, ColumnMetadata metadata,
+ BloomFilterConfig indexConfig) {
+ return BloomFilterReaderFactory.getBloomFilterReader(dataBuffer, indexConfig.isLoadOnHeap());
Review Comment:
Can BloomFilterReaderFactory itself be an IndexReaderFactory implementation?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1137262479
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/StandardIndexes.java:
##########
@@ -0,0 +1,39 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+
+
+@SuppressWarnings("unchecked")
+public class StandardIndexes {
Review Comment:
That is the same question @navina make below, right? It is explained in the javadoc I've added after that
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #10192: Index spi: index service
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1139856982
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/bloom/BloomIndexType.java:
##########
@@ -0,0 +1,113 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.local.segment.index.bloom;
+
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.segment.creator.impl.bloom.OnHeapGuavaBloomFilterCreator;
+import org.apache.pinot.segment.local.segment.index.readers.bloom.BloomFilterReaderFactory;
+import org.apache.pinot.segment.spi.ColumnMetadata;
+import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.V1Constants;
+import org.apache.pinot.segment.spi.creator.IndexCreationContext;
+import org.apache.pinot.segment.spi.index.FieldIndexConfigs;
+import org.apache.pinot.segment.spi.index.IndexHandler;
+import org.apache.pinot.segment.spi.index.IndexReaderFactory;
+import org.apache.pinot.segment.spi.index.IndexType;
+import org.apache.pinot.segment.spi.index.creator.BloomFilterCreator;
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.segment.spi.memory.PinotDataBuffer;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.Schema;
+
+public class BloomIndexType implements IndexType<BloomFilterConfig, BloomFilterReader, BloomFilterCreator> {
+ public static final BloomIndexType INSTANCE = new BloomIndexType();
+
+ @Override
+ public String getId() {
+ return "bloom_filter";
Review Comment:
Can this id be class name or a public static final string? So it is easier for cross reference?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] navina commented on a diff in pull request #10192: Index spi: index service
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136578756
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
Review Comment:
These comments will be useful when added as a doc for `_instance` member variable.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
Review Comment:
When would we want to change the instance at runtime?
In general, it is simpler to maintain immutable instances of objects. So, I am wondering if and why there would be a need to change this instance at runtime.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexPlugin.java:
##########
@@ -0,0 +1,25 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+
+public interface IndexPlugin<T extends IndexType<?, ?, ?>> {
Review Comment:
Can you add javadocs on what this interface is and how it should be used? I think in the PEP you talked about this.
One thing that is not clear to me is: Does every IndexType require an implementation of this interface? If yes, what is the benefit of doing so?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
+
+ private static volatile IndexService _instance = fromServiceLoader();
+
+ private final Set<IndexType<?, ?, ?>> _allIndexes;
+
+ public IndexService(Set<IndexPlugin<?>> allPlugins) {
+ _allIndexes = Sets.newHashSetWithExpectedSize(allPlugins.size());
+
+ for (IndexPlugin<?> plugin : allPlugins) {
+ _allIndexes.add(plugin.getIndexType());
+ }
+ }
+
+ public static IndexService getInstance() {
+ return _instance;
+ }
+
+ public static void setInstance(IndexService other) {
+ _instance = other;
+ }
+
+ public static IndexService fromServiceLoader() {
+ Set<IndexPlugin<?>> pluginList = new HashSet<>();
+ for (IndexPlugin indexPlugin : ServiceLoader.load(IndexPlugin.class)) {
+ pluginList.add(indexPlugin);
+ }
+ return new IndexService(pluginList);
+ }
+
+ public Set<IndexType<?, ?, ?>> getAllIndexes() {
+ return _allIndexes;
+ }
+
+ public Optional<IndexType<?, ?, ?>> getIndexTypeById(String indexId) {
+ return getAllIndexes().stream().filter(indexType -> indexType.getId().equalsIgnoreCase(indexId)).findAny();
+ }
+
+ public IndexType<?, ?, ?> getIndexTypeByIdOrThrow(String indexId) {
+ return getIndexTypeById(indexId)
+ .orElseThrow(() -> new IllegalArgumentException("Unknown index id: " + indexId));
+ }
Review Comment:
What are the allowed values for the `indexId` string? Can you add javadocs to these methods?
Do we really need both methods - one that throws and the one which doesn't?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/StandardIndexes.java:
##########
@@ -0,0 +1,39 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+
+
+@SuppressWarnings("unchecked")
+public class StandardIndexes {
Review Comment:
What does Standard Indexes mean? Can you add docs ?
##########
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:
If it is disabled, shouldn't the other member variables like `_fpp`, `_maxSizeInBytes` and `_loadOnHeap` set to invalid values??
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
+
+ private static volatile IndexService _instance = fromServiceLoader();
+
+ private final Set<IndexType<?, ?, ?>> _allIndexes;
+
+ public IndexService(Set<IndexPlugin<?>> allPlugins) {
Review Comment:
I am assuming the `IndexService` will always be created using the serviceLoader. Thus, this constructor can actually be `private` unless there is a specific reason / use-case to keep this at public scope
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1137026823
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
Review Comment:
BTW, we already discussed abut the (ab)use of singletons and static variables in the PEP: https://docs.google.com/document/d/1L_GJ2tAgZVxI_Ssa59--Mso3mK_HeAS7s7G9261-sOE/edit?disco=AAAAsjZUDEA
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] navina commented on a diff in pull request #10192: Index spi: index service
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1139440322
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
+
+ private static volatile IndexService _instance = fromServiceLoader();
+
+ private final Set<IndexType<?, ?, ?>> _allIndexes;
+
+ public IndexService(Set<IndexPlugin<?>> allPlugins) {
Review Comment:
@gortiz For special distributions use-case that you have explained above (which doesn't yet exist btw), it is still a hassle for the caller to load the filtered list of plugins. For example, in order to disable one index plugin, say bloom filter, the caller will be forced to enumerate and load all other `IndexPlugin` (by service loader or other means) and then, invoke ctor.
```
Set<IndexPlugin<?>> myPlugins = new HashSet<>();
for (IndexPlugin indexPlugin : ServiceLoader.load(IndexPlugin.class)) {
if (indexPlugin.getType().equals("bloom_filter")) {
continue;
}
pluginList.add(indexPlugin);
}
```
We can provide more helper util methods for discovery and loading `IndexPlugin`. For example:
```
public static Set<IndexPlugin> loadWithServiceLoader() {
...
}
```
Then, the only entry point for creating this IndexService instance would be via the ctor. Basically, leave it entirely up-to the caller to decide the list of IndexPlugin and only provide helper utils to discover the standard index plugins etc.
This is an option.
Otherwise, since currently there is no active use of the public ctor, it seems odd to keep it public when there is a static method to instantiate.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1137271683
##########
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);
Review Comment:
We could do that, but doesn't seem necessary. Anyway, don't study too much the bloom related things in this PR, as most indexes are going to be added [here](https://github.com/apache/pinot/pull/10193) and they wont be used until https://github.com/apache/pinot/pull/10194 and specially https://github.com/apache/pinot/pull/10184.
The reason to include bloom stuff here is to show an implementation example
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] saurabhd336 commented on a diff in pull request #10192: Index spi: index service
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136654762
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/bloom/BloomIndexType.java:
##########
@@ -0,0 +1,109 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.local.segment.index.bloom;
+
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.segment.creator.impl.bloom.OnHeapGuavaBloomFilterCreator;
+import org.apache.pinot.segment.local.segment.index.readers.bloom.BloomFilterReaderFactory;
+import org.apache.pinot.segment.spi.ColumnMetadata;
+import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.V1Constants;
+import org.apache.pinot.segment.spi.creator.IndexCreationContext;
+import org.apache.pinot.segment.spi.index.FieldIndexConfigs;
+import org.apache.pinot.segment.spi.index.IndexHandler;
+import org.apache.pinot.segment.spi.index.IndexReaderFactory;
+import org.apache.pinot.segment.spi.index.IndexType;
+import org.apache.pinot.segment.spi.index.creator.BloomFilterCreator;
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.segment.spi.memory.PinotDataBuffer;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.Schema;
+
+public class BloomIndexType implements IndexType<BloomFilterConfig, BloomFilterReader, BloomFilterCreator> {
+ public static final BloomIndexType INSTANCE = new BloomIndexType();
+
+ @Override
+ public String getId() {
+ return "bloom_filter";
+ }
+
+ @Override
+ public Class<BloomFilterConfig> getIndexConfigClass() {
+ return BloomFilterConfig.class;
+ }
+
+ @Override
+ public BloomFilterConfig getDefaultConfig() {
+ return BloomFilterConfig.DISABLED;
+ }
+
+ @Override
+ public BloomFilterConfig getConfig(TableConfig tableConfig, Schema schema) {
+ throw new UnsupportedOperationException("To be implemented in a future PR");
+ }
+
+ @Override
+ public BloomFilterCreator createIndexCreator(IndexCreationContext context, BloomFilterConfig indexConfig) {
+ int cardinality = context.getCardinality();
+ if (cardinality == Constants.UNKNOWN_CARDINALITY) {
+ // This is when we're creating bloom filters for non dictionary encoded cols where exact cardinality is not
+ // known beforehand.
+ // Since this field is only used for the estimate cardinality, using total # of entries instead
+ // TODO (saurabh) Check if we can do a better estimate
+ cardinality = context.getTotalNumberOfEntries();
+ }
+ return new OnHeapGuavaBloomFilterCreator(context.getIndexDir(), context.getFieldSpec().getName(), cardinality,
+ indexConfig, context.getFieldSpec().getDataType());
+ }
+
+ @Override
+ public IndexReaderFactory<BloomFilterReader> getReaderFactory() {
+ return new IndexReaderFactory.Default<BloomFilterConfig, BloomFilterReader>() {
Review Comment:
Why do we need a new instance of the Factory on each call? Could this be a static Factory instance shared across calls?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136862867
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
Review Comment:
TL;DR: I think it is better to keep it here, but I don't have an strong opinion about that and we can move it if consensus.
Why I think it is better to be here:
I don't like the idea of adding these comments into a private attribute. Sure, it may be slightly better for someone that reads the class, but these lines may also be useful for callers. I would expect callers ask why they have to use this class by doing `IndexService.getInstance().getAllIndexes()`, which is quite verbose, instead of simply calling `IndexService.getAllIndexes()`. Well, they can answer that question is that we hope that, in a (probably far) future, you could simply inject the IndexService instance in your constructor. A caller can understand that if they open the Javadoc of `IndexService` in their own IDE without actually needed to read the source code of `IndexService`.
If we move these lines to the attribute javadoc instead of the class javadoc, the caller would need to open the class, hope that they have the sources downloaded and then go to some private attribute they don't actually know their existence in order to read that.
At the end of the day this comment mixes information related to the usability (why we decided to show this interface) and the implementation (why we didn't use other ways). The former is interesting for callers and therefore should be attached to some public concept (like the class or the getInstance method) and the later is interesting for the implementator (and therefore may be attached to the private attribute).
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136986561
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexPlugin.java:
##########
@@ -0,0 +1,25 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+
+public interface IndexPlugin<T extends IndexType<?, ?, ?>> {
Review Comment:
IndexPlugin is the way indexes are discovered. I preferred to separate the responsibilities in different classes, but the implementator can have both in the same class by implementing both IndexType and IndexPlugin.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1138573587
##########
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);
Review Comment:
Changed
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1138354380
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java:
##########
@@ -47,9 +48,18 @@ public class OnHeapGuavaBloomFilterCreator implements BloomFilterCreator {
private final File _bloomFilterFile;
private final BloomFilter<String> _bloomFilter;
+ private final FieldSpec.DataType _dataType;
+ // TODO: This method is here for compatibility reasons, should be removed in future PRs
Review Comment:
I see your point, but I don't actually know when it could be removed. There should be no usages of this method once the complete PR is merged, but I kept it there just in case some people out there is using that constructor. We can simply remove it if you prefer.
What I'm going to do is to change the comment into something like: `exit_criteria: remove once #10184 is merged` (https://github.com/apache/pinot/pull/10184 is the complete PR)
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136993116
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/StandardIndexes.java:
##########
@@ -0,0 +1,39 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+
+
+@SuppressWarnings("unchecked")
+public class StandardIndexes {
Review Comment:
Added. Also, if you want to see the actual use cases, they are in https://github.com/apache/pinot/pull/10194
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] saurabhd336 commented on pull request #10192: Index spi: index service
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on PR #10192:
URL: https://github.com/apache/pinot/pull/10192#issuecomment-1473153151
> qq @gortiz I don't see any changes being made to BloomFilterHandler.java in this PR. Will all the IndexHandler classes continue to directly create the corresponding creator instances? Can they follow the IndexService -> indexType -> Creator path too? Or are there challenges with that?
Never mind looks like the original PR https://github.com/apache/pinot/pull/10184 has those changes
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1139890165
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/bloom/BloomIndexType.java:
##########
@@ -0,0 +1,113 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.local.segment.index.bloom;
+
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.segment.creator.impl.bloom.OnHeapGuavaBloomFilterCreator;
+import org.apache.pinot.segment.local.segment.index.readers.bloom.BloomFilterReaderFactory;
+import org.apache.pinot.segment.spi.ColumnMetadata;
+import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.V1Constants;
+import org.apache.pinot.segment.spi.creator.IndexCreationContext;
+import org.apache.pinot.segment.spi.index.FieldIndexConfigs;
+import org.apache.pinot.segment.spi.index.IndexHandler;
+import org.apache.pinot.segment.spi.index.IndexReaderFactory;
+import org.apache.pinot.segment.spi.index.IndexType;
+import org.apache.pinot.segment.spi.index.creator.BloomFilterCreator;
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.segment.spi.memory.PinotDataBuffer;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.Schema;
+
+public class BloomIndexType implements IndexType<BloomFilterConfig, BloomFilterReader, BloomFilterCreator> {
+ public static final BloomIndexType INSTANCE = new BloomIndexType();
+
+ @Override
+ public String getId() {
+ return "bloom_filter";
Review Comment:
We have changed that in https://github.com/apache/pinot/pull/10193.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1137026823
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
Review Comment:
BTW, we already discussed abut the (ab)use of singletons and static variables in PEP document ([link](https://docs.google.com/document/d/1L_GJ2tAgZVxI_Ssa59--Mso3mK_HeAS7s7G9261-sOE/edit?disco=AAAAsjZUDEA))
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #10192:
URL: https://github.com/apache/pinot/pull/10192#issuecomment-1469983192
I've modified the description of the PR by adding a class diagram. I hope it can be useful
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1138538666
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
Review Comment:
> Curious if you had considered introducing this PinotContext object as part of index spi changes. Guessing it will be a lot more disruptive changes to the code?
In general I tried to reduce the changes as much as possible and specially do not introduce new concepts or mechanics. I think we can think about it in another PEP. Something that may be less invasive that introducing DI may be to use thread locals, which is not great, but at least doesn't require to modify that much code.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1137258477
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
+
+ private static volatile IndexService _instance = fromServiceLoader();
+
+ private final Set<IndexType<?, ?, ?>> _allIndexes;
+
+ public IndexService(Set<IndexPlugin<?>> allPlugins) {
+ _allIndexes = Sets.newHashSetWithExpectedSize(allPlugins.size());
+
+ for (IndexPlugin<?> plugin : allPlugins) {
+ _allIndexes.add(plugin.getIndexType());
+ }
+ }
+
+ public static IndexService getInstance() {
+ return _instance;
+ }
+
+ public static void setInstance(IndexService other) {
+ _instance = other;
+ }
+
+ public static IndexService fromServiceLoader() {
+ Set<IndexPlugin<?>> pluginList = new HashSet<>();
+ for (IndexPlugin indexPlugin : ServiceLoader.load(IndexPlugin.class)) {
+ pluginList.add(indexPlugin);
+ }
+ return new IndexService(pluginList);
+ }
+
+ public Set<IndexType<?, ?, ?>> getAllIndexes() {
+ return _allIndexes;
+ }
+
+ public Optional<IndexType<?, ?, ?>> getIndexTypeById(String indexId) {
+ return getAllIndexes().stream().filter(indexType -> indexType.getId().equalsIgnoreCase(indexId)).findAny();
Review Comment:
No, it cannot be an enum. Enums are by definition a closed set of elements decided at compile time. The idea behind `index-spi` is to not define the indexes at compile time but at startup time. Therefore enums cannot be used.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] saurabhd336 commented on pull request #10192: Index spi: index service
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on PR #10192:
URL: https://github.com/apache/pinot/pull/10192#issuecomment-1473144072
qq @gortiz I don't see any changes being made to BloomFilterHandler.java in this PR. Will all the IndexHandler classes continue to directly create the corresponding creator instances? Can they follow the IndexService -> indexType -> Creator path too? Or are there challenges with that?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1138535350
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
Review Comment:
I've made some changes to the javadoc. I hope it is better now! :)
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136809377
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
+
+ private static volatile IndexService _instance = fromServiceLoader();
+
+ private final Set<IndexType<?, ?, ?>> _allIndexes;
+
+ public IndexService(Set<IndexPlugin<?>> allPlugins) {
Review Comment:
I can imagine that most times, in production, IndexService is going to be created using the `ServiceLoader`. But I can imagine several scenarios where that is not going to happen:
1. Tests: If we want to test how some code behaves in case some index type is not present, if we don't have this method we either has to create our own classpath (which is tricky) or heavily use mocks (which is also tricky). Instead, with this constructor we can create our own `IndexService` with a limited number of index types.
2. Special distributions: The default idea of enabling all index types in the classpath seems correct for most distributions, but we can imagine a case when even if there is an index in the classpath, we want to disable it. For example, we could add an startup property with a list that filters-in or filters-out some index types. In that case, we should call this constructor at startup.
* For example, a customer don't want to use a specific index type because they know that it may produce problems in their environment. If the customer is not that large, they can decide to do not use it and that's it. But if the customer is large and it is difficult to coordinate all people that creates tables, they may decide to do not load at that index at startup time.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1136746523
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/bloom/BloomIndexType.java:
##########
@@ -0,0 +1,109 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.local.segment.index.bloom;
+
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.segment.creator.impl.bloom.OnHeapGuavaBloomFilterCreator;
+import org.apache.pinot.segment.local.segment.index.readers.bloom.BloomFilterReaderFactory;
+import org.apache.pinot.segment.spi.ColumnMetadata;
+import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.V1Constants;
+import org.apache.pinot.segment.spi.creator.IndexCreationContext;
+import org.apache.pinot.segment.spi.index.FieldIndexConfigs;
+import org.apache.pinot.segment.spi.index.IndexHandler;
+import org.apache.pinot.segment.spi.index.IndexReaderFactory;
+import org.apache.pinot.segment.spi.index.IndexType;
+import org.apache.pinot.segment.spi.index.creator.BloomFilterCreator;
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.segment.spi.memory.PinotDataBuffer;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.Schema;
+
+public class BloomIndexType implements IndexType<BloomFilterConfig, BloomFilterReader, BloomFilterCreator> {
+ public static final BloomIndexType INSTANCE = new BloomIndexType();
+
+ @Override
+ public String getId() {
+ return "bloom_filter";
+ }
+
+ @Override
+ public Class<BloomFilterConfig> getIndexConfigClass() {
+ return BloomFilterConfig.class;
+ }
+
+ @Override
+ public BloomFilterConfig getDefaultConfig() {
+ return BloomFilterConfig.DISABLED;
+ }
+
+ @Override
+ public BloomFilterConfig getConfig(TableConfig tableConfig, Schema schema) {
+ throw new UnsupportedOperationException("To be implemented in a future PR");
+ }
+
+ @Override
+ public BloomFilterCreator createIndexCreator(IndexCreationContext context, BloomFilterConfig indexConfig) {
+ int cardinality = context.getCardinality();
+ if (cardinality == Constants.UNKNOWN_CARDINALITY) {
+ // This is when we're creating bloom filters for non dictionary encoded cols where exact cardinality is not
+ // known beforehand.
+ // Since this field is only used for the estimate cardinality, using total # of entries instead
+ // TODO (saurabh) Check if we can do a better estimate
+ cardinality = context.getTotalNumberOfEntries();
+ }
+ return new OnHeapGuavaBloomFilterCreator(context.getIndexDir(), context.getFieldSpec().getName(), cardinality,
+ indexConfig, context.getFieldSpec().getDataType());
+ }
+
+ @Override
+ public IndexReaderFactory<BloomFilterReader> getReaderFactory() {
+ return new IndexReaderFactory.Default<BloomFilterConfig, BloomFilterReader>() {
Review Comment:
We don't _need_ to create a new instance each type, as they are stateless. It was a matter of simplicity in the code.
To be honest I don't think that really matters. This instance is stateless, is going to live a very short time and is going to be called once per segment and query. It is very probable that it is going to be optimized by the JVM and do not actually be allocated in the heap. And even if it does, it is going to very rapidly be collected by the young GC.
Given all of that, I would prefer to just let the code here in order to make it easier to read, but if there is a majority that think that it is better to catch it, I can modify it
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #10192: Index spi: index service
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1137989320
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexPlugin.java:
##########
@@ -0,0 +1,35 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+/**
+ * IndexPlugins are the way {@link IndexType}s are registered in a {@link IndexService}.
+ *
+ * In order to create an IndexService, a set of IndexPlugin must be provided. Although IndexTypes could be directly
+ * added into a IndexService, this indirection is used to decouple the way indexes are discovered
+ * (usually by {@link java.util.ServiceLoader} services) and the actual implementation.
+ *
+ * In order to mark a class as a {@link java.util.ServiceLoader} service, some metadata has to be added. Java modules
+ * define a typesafe way to define services, but given that Pinot does not use them right now, the easier way to create
Review Comment:
I am not very familiar with the ServiceLoader. but say in the future, we want to use the type. Will that be a problem?
What's the implication of using Google AutoService? Compared with the other type safe method.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] navina commented on a diff in pull request #10192: Index spi: index service
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1137597984
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
Review Comment:
Thanks for the detailed explanation. I unfortunately agree with the lack of a contextual instance in the architecture. I think we had discussed on similar lines in my metrics registry for plugins.
Glad that you have thought this through and had to make this choice :)
Curious if you had considered introducing this PinotContext object as part of index spi changes. Guessing it will be a lot more disruptive changes to the code?
nit: Please add javadoc / annotation about the need for thread safety guarantees for implementations of this interface
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
+
+ private static volatile IndexService _instance = fromServiceLoader();
+
+ private final Set<IndexType<?, ?, ?>> _allIndexes;
+
+ public IndexService(Set<IndexPlugin<?>> allPlugins) {
+ _allIndexes = Sets.newHashSetWithExpectedSize(allPlugins.size());
+
+ for (IndexPlugin<?> plugin : allPlugins) {
+ _allIndexes.add(plugin.getIndexType());
+ }
+ }
+
+ public static IndexService getInstance() {
+ return _instance;
+ }
+
+ public static void setInstance(IndexService other) {
+ _instance = other;
+ }
+
+ public static IndexService fromServiceLoader() {
+ Set<IndexPlugin<?>> pluginList = new HashSet<>();
+ for (IndexPlugin indexPlugin : ServiceLoader.load(IndexPlugin.class)) {
+ pluginList.add(indexPlugin);
+ }
+ return new IndexService(pluginList);
+ }
+
+ public Set<IndexType<?, ?, ?>> getAllIndexes() {
+ return _allIndexes;
+ }
+
+ public Optional<IndexType<?, ?, ?>> getIndexTypeById(String indexId) {
+ return getAllIndexes().stream().filter(indexType -> indexType.getId().equalsIgnoreCase(indexId)).findAny();
+ }
+
+ public IndexType<?, ?, ?> getIndexTypeByIdOrThrow(String indexId) {
+ return getIndexTypeById(indexId)
+ .orElseThrow(() -> new IllegalArgumentException("Unknown index id: " + indexId));
+ }
Review Comment:
I wouldn't go with 1. as returning `null` has been plaguing the codebase and not a good practice anyway. I don't like option 2 as it requires the caller to handle exceptions and this is a method that might be used frequently in many parts of the codebase.
I didn't think the overhead with optional (in Option #3) would be that much of a problem.
I like your last proposal - `get` -> throws if IndexType is not there; `getOptional` -> may return an optional reference.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
Review Comment:
I think it is well-known to a Pinot developer that we don't use a standard injection mechanism across the codebase. My comment was specifically for these 2 lines:
```Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.```
It is not clear what static field you are talking about. When the interface evolves in the future, this comment might no longer be relevant.
Imo, it seems out of place as it doesn't talk about the usability of the interface. It pertains to the current choice of implementation that is not required for any caller or even implementer to know because it is well-known to a developer that Pinot doesn't use a standard injection mechanism across the codebase.
If you want to emphasize the usability of this interface, you should just literally explain how to use it in the javadoc. The future improvements can be annotated as a TODO or Note.
An example could be:
```
This is the entry point of the Index SPI.
By default, this service will be initialized by reading all ServiceLoader SPI services
that implement {@link IndexPlugin}, adding all the {@link IndexType} that can be found in
that way. A caller can iterate it by invoking `IndexService.getInstance().getAllIndexes()`
The interface allows changing the singleton instance at runtime using the method {@link #setInstance(IndexService)}.
Note: Since Pinot doesn't support injection mechanism, we have to rely on static fields (singleton pattern) like `_instance`. With an injection mechanism, this instance can be part of a `PinotContext` object that is accessible in all components.
```
I still think those 2 lines in the javadoc are out of place and me, as a pinot developer, don't find it useful. Will let others chime in.
##########
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:
makes sense. Agree it is not always possible to assign a default / invalid value for a type. But it wasn't obvious from reading the code that the caller should always check if an index is enabled or not before trying to fetch the config values.
It's fine for now. Will probably become easier to understand as index-spi feature implementation continues.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1138350647
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexPlugin.java:
##########
@@ -0,0 +1,35 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+/**
+ * IndexPlugins are the way {@link IndexType}s are registered in a {@link IndexService}.
+ *
+ * In order to create an IndexService, a set of IndexPlugin must be provided. Although IndexTypes could be directly
+ * added into a IndexService, this indirection is used to decouple the way indexes are discovered
+ * (usually by {@link java.util.ServiceLoader} services) and the actual implementation.
+ *
+ * In order to mark a class as a {@link java.util.ServiceLoader} service, some metadata has to be added. Java modules
+ * define a typesafe way to define services, but given that Pinot does not use them right now, the easier way to create
Review Comment:
Historically, in order to create a ServiceLoader service you need to add some text in the MANIFES file all jars usually have. This is tedious, error prone and personally I think that is one of the reasons most people don't know about ServiceLoader system. This example from [baeldung](https://www.baeldung.com/java-spi) shows how to manually create a ServiceLoader service. There is a new typesafe mechanism that was added when using Java modules. Given that we don't use it, I'm not going to explain that much about it, but it is important to say that if someday use Java modules, both ways to declare services are compatible.
[Baeldung](https://www.baeldung.com/google-autoservice) also has another tutorial showing how to use Google AutoServe.
What Google AutoService does is a very simple trick based on standards:
- You annotate a class with `@AutoService(WhateverInterfaceTheClassImplement.class)`
- AutoService uses a Java preprocessor, which is a standard `javac` feature that offers hooks where your code can apply. What the AutoService Java preprocessor does is trivial: It just generate the entry in the MANIFEST.MF file you would need to add manually instead.
- Given that this is only done in compile time, you don't even need to have the AutoService dependency at runtime.
So there is no magic here, no reflection, anything. It is just a typesafe way to generate an entry in the MANIFEST without having to add it manually.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1138553710
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
+
+ private static volatile IndexService _instance = fromServiceLoader();
+
+ private final Set<IndexType<?, ?, ?>> _allIndexes;
+
+ public IndexService(Set<IndexPlugin<?>> allPlugins) {
+ _allIndexes = Sets.newHashSetWithExpectedSize(allPlugins.size());
+
+ for (IndexPlugin<?> plugin : allPlugins) {
+ _allIndexes.add(plugin.getIndexType());
+ }
+ }
+
+ public static IndexService getInstance() {
+ return _instance;
+ }
+
+ public static void setInstance(IndexService other) {
+ _instance = other;
+ }
+
+ public static IndexService fromServiceLoader() {
+ Set<IndexPlugin<?>> pluginList = new HashSet<>();
+ for (IndexPlugin indexPlugin : ServiceLoader.load(IndexPlugin.class)) {
+ pluginList.add(indexPlugin);
+ }
+ return new IndexService(pluginList);
+ }
+
+ public Set<IndexType<?, ?, ?>> getAllIndexes() {
+ return _allIndexes;
+ }
+
+ public Optional<IndexType<?, ?, ?>> getIndexTypeById(String indexId) {
+ return getAllIndexes().stream().filter(indexType -> indexType.getId().equalsIgnoreCase(indexId)).findAny();
+ }
+
+ public IndexType<?, ?, ?> getIndexTypeByIdOrThrow(String indexId) {
+ return getIndexTypeById(indexId)
+ .orElseThrow(() -> new IllegalArgumentException("Unknown index id: " + indexId));
+ }
Review Comment:
Ok, I'll change it to do that.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1138538666
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
Review Comment:
> Curious if you had considered introducing this PinotContext object as part of index spi changes. Guessing it will be a lot more disruptive changes to the code?
In general I tried to reduce the changes as much as possible and specially do not introduce new concepts or mechanics. I think we can think about it in another PEP. Something that may be less invasive that introducing DI may be to use thread locals, which is not great, but at least doesn't require to modify that much code.
BTW: If javadoc is better/acceptable now, please resolve the conversation, otherwise don't hesitate to tell me.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1138550323
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
+
+ private static volatile IndexService _instance = fromServiceLoader();
+
+ private final Set<IndexType<?, ?, ?>> _allIndexes;
+
+ public IndexService(Set<IndexPlugin<?>> allPlugins) {
Review Comment:
> Otherwise, some code outside index service needs to handle the customized logic. If we take a configuration, by default, it loads from service loader. Otherwise, it builds based on configuration.
I don't get it. That is precisely the idea. The IndexService is config agnostic. Someone else (usually Pinot startup) is the one that configures it based on the PinotConfiguration received.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] navina commented on a diff in pull request #10192: Index spi: index service
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1137789064
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java:
##########
@@ -47,9 +48,18 @@ public class OnHeapGuavaBloomFilterCreator implements BloomFilterCreator {
private final File _bloomFilterFile;
private final BloomFilter<String> _bloomFilter;
+ private final FieldSpec.DataType _dataType;
+ // TODO: This method is here for compatibility reasons, should be removed in future PRs
Review Comment:
nit: In my experience, it is better to define an "exit criteria" for the deprecated feature. Meaning "when" in the future can this be safely removed. In my previous team, we used something like:
```
exit_criteria = "when PR #XYZ is merged in oss"
```
In this case, you can mention when it will no longer matter. It can also be as straightforward as "remove in releaseX.Y.Z"
I don't know if Pinot has a standard practice for removing deprecated feature/code. But just my 2 cents.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/bloom/BloomIndexType.java:
##########
@@ -0,0 +1,109 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.local.segment.index.bloom;
+
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.segment.creator.impl.bloom.OnHeapGuavaBloomFilterCreator;
+import org.apache.pinot.segment.local.segment.index.readers.bloom.BloomFilterReaderFactory;
+import org.apache.pinot.segment.spi.ColumnMetadata;
+import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.V1Constants;
+import org.apache.pinot.segment.spi.creator.IndexCreationContext;
+import org.apache.pinot.segment.spi.index.FieldIndexConfigs;
+import org.apache.pinot.segment.spi.index.IndexHandler;
+import org.apache.pinot.segment.spi.index.IndexReaderFactory;
+import org.apache.pinot.segment.spi.index.IndexType;
+import org.apache.pinot.segment.spi.index.creator.BloomFilterCreator;
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.segment.spi.memory.PinotDataBuffer;
+import org.apache.pinot.segment.spi.store.SegmentDirectory;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.Schema;
+
+public class BloomIndexType implements IndexType<BloomFilterConfig, BloomFilterReader, BloomFilterCreator> {
+ public static final BloomIndexType INSTANCE = new BloomIndexType();
+
+ @Override
+ public String getId() {
+ return "bloom_filter";
+ }
+
+ @Override
+ public Class<BloomFilterConfig> getIndexConfigClass() {
+ return BloomFilterConfig.class;
+ }
+
+ @Override
+ public BloomFilterConfig getDefaultConfig() {
+ return BloomFilterConfig.DISABLED;
+ }
+
+ @Override
+ public BloomFilterConfig getConfig(TableConfig tableConfig, Schema schema) {
+ throw new UnsupportedOperationException("To be implemented in a future PR");
+ }
+
+ @Override
+ public BloomFilterCreator createIndexCreator(IndexCreationContext context, BloomFilterConfig indexConfig) {
+ int cardinality = context.getCardinality();
+ if (cardinality == Constants.UNKNOWN_CARDINALITY) {
+ // This is when we're creating bloom filters for non dictionary encoded cols where exact cardinality is not
+ // known beforehand.
+ // Since this field is only used for the estimate cardinality, using total # of entries instead
+ // TODO (saurabh) Check if we can do a better estimate
+ cardinality = context.getTotalNumberOfEntries();
+ }
+ return new OnHeapGuavaBloomFilterCreator(context.getIndexDir(), context.getFieldSpec().getName(), cardinality,
+ indexConfig, context.getFieldSpec().getDataType());
+ }
+
+ @Override
+ public IndexReaderFactory<BloomFilterReader> getReaderFactory() {
+ return new IndexReaderFactory.Default<BloomFilterConfig, BloomFilterReader>() {
Review Comment:
+1 to @saurabhd336 's comment.
Creating a new instance for what is meant to be a stateless instance is a little unnecessary. I agree that GC will take care of it. But it may not be obvious to every developer. So, better to move it to a single instance and share it.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #10192: Index spi: index service
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1137993277
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/StandardIndexes.java:
##########
@@ -0,0 +1,39 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import org.apache.pinot.segment.spi.index.reader.BloomFilterReader;
+import org.apache.pinot.spi.config.table.BloomFilterConfig;
+
+
+@SuppressWarnings("unchecked")
+public class StandardIndexes {
Review Comment:
yeah. sorry, didn't see that
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] npawar merged pull request #10192: Index spi: index service
Posted by "npawar (via GitHub)" <gi...@apache.org>.
npawar merged PR #10192:
URL: https://github.com/apache/pinot/pull/10192
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10192: Index spi: index service
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10192:
URL: https://github.com/apache/pinot/pull/10192#discussion_r1139887491
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/IndexService.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.segment.spi.index;
+
+import com.google.common.collect.Sets;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.ServiceLoader;
+import java.util.Set;
+
+
+/**
+ * This is the entry point of the Index SPI.
+ *
+ * Ideally, if we used some kind of injection system, this class should be injected into a Pinot context all classes can
+ * receive when they are built. Given that Pinot doesn't have that, we have to relay on static fields.
+ *
+ * By default, this class will be initialized by reading all ServiceLoader SPI services that implement
+ * {@link IndexPlugin}, adding all the {@link IndexType} that can be found in that way.
+ *
+ * In case we want to change the instance to be used at runtime, the method {@link #setInstance(IndexService)} can be
+ * called.
+ */
+public class IndexService {
+
+ private static volatile IndexService _instance = fromServiceLoader();
+
+ private final Set<IndexType<?, ?, ?>> _allIndexes;
+
+ public IndexService(Set<IndexPlugin<?>> allPlugins) {
Review Comment:
I really don't know why do you feel so uncomfortable about having a public constructor. We have tons of classes with public constructors without being a problem. I think it will be very useful for several cases we don't have right now and I would even create a new one that directly receives the set of indexes instead of set of plugins in order to make your case easier to implement.
But given that we are not using it right now and it is clear some of you don't like the idea of having it, I'm going to make it private. In case we need it in future, we can add just make it public or create a static factory as suggested by @61yao.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org