[GitHub] [pinot] sajjad-moradi commented on a change in pull request #8279: Make realtime indexes pluggable

[GitBox <gi...@apache.org>]

sajjad-moradi commented on a change in pull request #8279:
URL: https://github.com/apache/pinot/pull/8279#discussion_r819789995



##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/mutable/provider/MutableForwardIndexProvider.java
##########
@@ -0,0 +1,26 @@
+/**
+ * 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.mutable.provider;
+
+import org.apache.pinot.segment.spi.index.mutable.MutableForwardIndex;
+
+
+public interface MutableForwardIndexProvider {
+  MutableForwardIndex newIndex(MutableIndexContext.Forward context);

Review comment:
       This is subjective, but IMO `newForwardIndex` reads better and less ambiguous. It's clear here in the interface definition because of the parameter type, but it will be a bit ambiguous in places where it's being used and the parameter name doesn't have word forward in it - like the one you have here `context`.

##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
##########
@@ -242,6 +235,10 @@ public long getLatestIngestionTimestamp() {
     // Initialize for each column
     for (FieldSpec fieldSpec : _physicalFieldSpecs) {
       String column = fieldSpec.getName();
+      boolean isRaw = isNoDictionaryColumn(noDictionaryColumns, invertedIndexColumns, fieldSpec, column);

Review comment:
       If you rename `isRaw` to `isNoDictionary`, you don't need to negate it in both places it's being used and IMO it reads better.

##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/mutable/provider/MutableIndexContext.java
##########
@@ -0,0 +1,249 @@
+/**
+ * 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.mutable.provider;
+
+import java.util.Objects;
+import org.apache.pinot.segment.spi.memory.PinotDataBufferMemoryManager;
+import org.apache.pinot.spi.data.FieldSpec;
+
+
+public interface MutableIndexContext {
+  PinotDataBufferMemoryManager getMemoryManager();
+
+  FieldSpec getFieldSpec();
+
+  String getSegmentName();
+
+  boolean hasDictionary();
+
+  int getCapacity();
+
+  boolean isOffHeap();
+
+  static Builder builder() {
+    return new Builder();
+  }
+
+  class Builder {
+    private FieldSpec _fieldSpec;
+    private String _segmentName;
+    private boolean _hasDictionary = true;
+    private boolean _offHeap = true;
+    private int _capacity;
+    private PinotDataBufferMemoryManager _memoryManager;
+
+    public Builder withMemoryManager(PinotDataBufferMemoryManager memoryManager) {
+      _memoryManager = memoryManager;
+      return this;
+    }
+
+    public Builder withFieldSpec(FieldSpec fieldSpec) {
+      _fieldSpec = fieldSpec;
+      return this;
+    }
+
+    public Builder withSegmentName(String segmentName) {
+      _segmentName = segmentName;
+      return this;
+    }
+
+    public Builder withDictionary(boolean hasDictionary) {
+      _hasDictionary = hasDictionary;
+      return this;
+    }
+
+    public Builder offHeap(boolean offHeap) {
+      _offHeap = offHeap;
+      return this;
+    }
+
+    public Builder withCapacity(int capacity) {
+      _capacity = capacity;
+      return this;
+    }
+
+    public Common build() {
+      return new Common(Objects.requireNonNull(_fieldSpec), _hasDictionary, Objects.requireNonNull(_segmentName),
+          Objects.requireNonNull(_memoryManager), _capacity, _offHeap);
+    }
+  }
+
+  final class Common implements MutableIndexContext {
+    private final int _capacity;
+    private final FieldSpec _fieldSpec;
+    private final boolean _hasDictionary;
+    private final boolean _offHeap;
+    private final String _segmentName;
+    private final PinotDataBufferMemoryManager _memoryManager;
+
+    public Common(FieldSpec fieldSpec, boolean hasDictionary, String segmentName,
+        PinotDataBufferMemoryManager memoryManager, int capacity, boolean offHeap) {
+      _fieldSpec = fieldSpec;
+      _hasDictionary = hasDictionary;
+      _segmentName = segmentName;
+      _memoryManager = memoryManager;
+      _capacity = capacity;
+      _offHeap = offHeap;
+    }
+
+    @Override
+    public PinotDataBufferMemoryManager getMemoryManager() {
+      return _memoryManager;
+    }
+
+    @Override
+    public String getSegmentName() {
+      return _segmentName;
+    }
+
+    @Override
+    public FieldSpec getFieldSpec() {
+      return _fieldSpec;
+    }
+
+    @Override
+    public boolean hasDictionary() {
+      return _hasDictionary;
+    }
+
+    @Override
+    public int getCapacity() {
+      return _capacity;
+    }
+
+    @Override
+    public boolean isOffHeap() {
+      return _offHeap;
+    }
+
+    public Dictionary forDictionary(int estimatedColSize, int estimatedCardinality) {
+      return new Dictionary(this, estimatedColSize, estimatedCardinality);
+    }
+
+    public Forward forForwardIndex(int avgNumMultiValues) {
+      return new Forward(this, avgNumMultiValues);
+    }
+
+    public Inverted forInvertedIndex() {
+      return new Inverted(this);
+    }
+
+    public Json forJsonIndex() {
+      return new Json(this);
+    }
+
+    public Text forTextIndex() {
+      return new Text(this);
+    }
+  }
+
+  class Wrapper implements MutableIndexContext {

Review comment:
       Why do you need Wrapper class? Dictionary, Inverted, Json, etc can directly extend Common!




-- 
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