klsince commented on code in PR #10184:
URL: https://github.com/apache/pinot/pull/10184#discussion_r1148051608
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/JsonIndexConfig.java:
##########
@@ -38,14 +39,25 @@
* to be excluded.
* - excludeFields: Exclude the given fields, e.g. "b", "c", even if it is
under the included paths.
*/
-public class JsonIndexConfig extends BaseJsonConfig {
+public class JsonIndexConfig extends IndexConfig {
Review Comment:
looks like those XXXIndexConfig.java files are spread in many pkgs, should
they be centralized?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/AbstractForwardIndexCreator.java:
##########
@@ -16,10 +16,16 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.pinot.segment.spi.index.reader.provider;
-public interface IndexReaderProvider
- extends BloomFilterReaderProvider, ForwardIndexReaderProvider,
GeospatialIndexReaderProvider,
- InvertedIndexReaderProvider, JsonIndexReaderProvider,
RangeIndexReaderProvider, SortedIndexReaderProvider,
- TextIndexReaderProvider {
+package org.apache.pinot.segment.local.segment.creator.impl.fwd;
+
+import java.io.IOException;
+import org.apache.pinot.segment.spi.index.creator.ForwardIndexCreator;
+
+
+public abstract class AbstractForwardIndexCreator implements
ForwardIndexCreator {
+ @Override
+ public void seal()
Review Comment:
add a default seal() in ForwardIndexCreator? and save changes in the next
few fwd index creator classes.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/H3IndexConfig.java:
##########
@@ -18,20 +18,38 @@
*/
package org.apache.pinot.segment.spi.index.creator;
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Preconditions;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
+import javax.annotation.Nullable;
import org.apache.commons.lang3.StringUtils;
import org.apache.pinot.segment.spi.index.reader.H3IndexResolution;
+import org.apache.pinot.spi.config.table.IndexConfig;
-public class H3IndexConfig {
+public class H3IndexConfig extends IndexConfig {
Review Comment:
Move this file into /index package where all other XXXConfig.java files are
kept?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/geospatial/BaseH3IndexCreator.java:
##########
@@ -101,6 +102,11 @@ public abstract class BaseH3IndexCreator implements
GeoSpatialIndexCreator {
_lowestResolution = resolution.getLowestResolution();
}
+ @Override
+ public Geometry deserialize(byte[] bytes) {
Review Comment:
for
```
_indexFile = new File(indexDir, columnName +
V1Constants.Indexes.H3_INDEX_FILE_EXTENSION);
```
in the BaseH3IndexCreator() above, should it use
H3IndexType.INSTANCE.getFileExtension() instead? and same for a few other index
creator classes below.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/text/NativeTextIndexCreator.java:
##########
@@ -73,7 +75,8 @@ public class NativeTextIndexCreator implements
TextIndexCreator {
private int _fstDataSize;
private int _numBitMaps;
- public NativeTextIndexCreator(String column, File indexDir)
+ public NativeTextIndexCreator(String column, File indexDir,
+ @Nullable Object rawValueForTextIndex, FieldSpec fieldSpec)
Review Comment:
no use of the two new params?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/TextIndexCreator.java:
##########
@@ -18,14 +18,22 @@
*/
package org.apache.pinot.segment.spi.index.creator;
-import java.io.Closeable;
import java.io.IOException;
+import org.apache.pinot.segment.spi.index.IndexCreator;
/**
- * Index creator for text index.
+ * Index creator for both text and FST indexes.
+ *
+ * This abstraction is not great. Text and FST indexes are quite different and
in fact they way they are created is not
Review Comment:
... in fact `the` way...
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/text/LuceneFSTIndexCreator.java:
##########
@@ -21,11 +21,14 @@
import java.io.File;
Review Comment:
how about add a FSTIndexCreator interface to separate FST and Text index
clearly.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/fwd/SameValueForwardIndexCreator.java:
##########
@@ -0,0 +1,137 @@
+/**
+ * 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.creator.impl.fwd;
+
+import java.io.IOException;
+import java.math.BigDecimal;
+import org.apache.pinot.spi.data.FieldSpec;
+
+
+public class SameValueForwardIndexCreator extends AbstractForwardIndexCreator {
Review Comment:
add a comment about this class, which seems only used for TextIndexCreator
as the fakeValueFwdCreator. If so, maybe make this an inner class or pkg
private class only available for TextIndexCreator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
