Jackie-Jiang commented on code in PR #18368:
URL: https://github.com/apache/pinot/pull/18368#discussion_r3255477904
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/datasource/MapDataSource.java:
##########
@@ -33,6 +33,19 @@ public interface MapDataSource extends DataSource {
/// Returns the DataSource for the given map key's values.
DataSource getDataSource(String key);
+ /// Returns whether this segment has per-key index data for the given key.
Columnar segments
+ /// return an exact answer (O(1) lookup into the materialized key set).
Blob-only segments
+ /// return {@code true} conservatively because determining key presence
requires deserialization.
+ ///
+ /// <p>Query operators use this to choose between fast-path (per-key
inverted/dictionary index)
Review Comment:
(minor) `<p>` is not required for markdown style
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ColumnarMapIndexReader.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * 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.reader;
+
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.spi.index.IndexReader;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.roaringbitmap.buffer.ImmutableRoaringBitmap;
+
+
+/// Reader for the MAP index. Each indexed key is materialized as its own
per-key
+/// forward index plus a presence bitmap.
+///
+/// Implementations must be safe for concurrent reads. Mutable implementations
may impose
+/// a single-writer constraint; refer to the concrete implementation's Javadoc
for details.
+///
+/// Per-key `DataSource` construction is the responsibility of the surrounding
+/// `ColumnarMapDataSource` wrappers, not this reader. This interface exposes
only
+/// the primitives a wrapper needs (key set, type, presence bitmap, per-doc
map view).
+public interface ColumnarMapIndexReader extends IndexReader {
+
+ /// Returns the set of all indexed key names. Never null; empty if no keys
are indexed.
+ Set<String> getKeys();
+
+ /// Returns the value DataType for the given key, or null if the key is not
indexed.
+ @Nullable
+ DataType getValueType(String key);
+
+ /// Returns the number of documents that have a non-null value for the given
key.
+ /// Returns 0 if the key is not indexed.
+ int getNumDocsWithKey(String key);
+
+ /// Returns the presence bitmap for the given key (docIds with non-null
values).
+ /// Returns an empty bitmap if the key is not indexed. The returned bitmap
must not be mutated.
+ ImmutableRoaringBitmap getPresenceBitmap(String key);
+
+ /// Reconstructs the full map for a single document from per-key data. Only
keys with a
+ /// non-null value at `docId` appear in the result. Returns an empty map if
the document has
+ /// no values; behavior for an out-of-range `docId` is
implementation-defined.
+ Map<String, Object> getMap(int docId);
+
+ /// Returns whether the given key has an inverted index available. False if
the key is not indexed.
+ default boolean hasInvertedIndex(String key) {
+ return false;
+ }
+
+ /// Returns sorted distinct values for the key from the inverted index, or
null if no
+ /// inverted index is available (or the key is not indexed).
+ @Nullable
+ default String[] getDistinctValuesForKey(String key) {
Review Comment:
Why is it from inverted index? Is this for dictionary build purpose?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ColumnarMapIndexReader.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * 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.reader;
+
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.spi.index.IndexReader;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.roaringbitmap.buffer.ImmutableRoaringBitmap;
+
+
+/// Reader for the MAP index. Each indexed key is materialized as its own
per-key
+/// forward index plus a presence bitmap.
+///
+/// Implementations must be safe for concurrent reads. Mutable implementations
may impose
+/// a single-writer constraint; refer to the concrete implementation's Javadoc
for details.
+///
+/// Per-key `DataSource` construction is the responsibility of the surrounding
+/// `ColumnarMapDataSource` wrappers, not this reader. This interface exposes
only
+/// the primitives a wrapper needs (key set, type, presence bitmap, per-doc
map view).
+public interface ColumnarMapIndexReader extends IndexReader {
+
+ /// Returns the set of all indexed key names. Never null; empty if no keys
are indexed.
+ Set<String> getKeys();
+
+ /// Returns the value DataType for the given key, or null if the key is not
indexed.
+ @Nullable
+ DataType getValueType(String key);
+
+ /// Returns the number of documents that have a non-null value for the given
key.
+ /// Returns 0 if the key is not indexed.
+ int getNumDocsWithKey(String key);
+
+ /// Returns the presence bitmap for the given key (docIds with non-null
values).
+ /// Returns an empty bitmap if the key is not indexed. The returned bitmap
must not be mutated.
+ ImmutableRoaringBitmap getPresenceBitmap(String key);
+
+ /// Reconstructs the full map for a single document from per-key data. Only
keys with a
+ /// non-null value at `docId` appear in the result. Returns an empty map if
the document has
+ /// no values; behavior for an out-of-range `docId` is
implementation-defined.
+ Map<String, Object> getMap(int docId);
+
+ /// Returns whether the given key has an inverted index available. False if
the key is not indexed.
+ default boolean hasInvertedIndex(String key) {
Review Comment:
Will the materialized key column be a column abstraction? Are you going to
support other indexes such as range, text etc?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/ColumnarMapIndexReader.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * 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.reader;
+
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.spi.index.IndexReader;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.roaringbitmap.buffer.ImmutableRoaringBitmap;
+
+
+/// Reader for the MAP index. Each indexed key is materialized as its own
per-key
+/// forward index plus a presence bitmap.
+///
+/// Implementations must be safe for concurrent reads. Mutable implementations
may impose
+/// a single-writer constraint; refer to the concrete implementation's Javadoc
for details.
+///
+/// Per-key `DataSource` construction is the responsibility of the surrounding
+/// `ColumnarMapDataSource` wrappers, not this reader. This interface exposes
only
+/// the primitives a wrapper needs (key set, type, presence bitmap, per-doc
map view).
+public interface ColumnarMapIndexReader extends IndexReader {
+
+ /// Returns the set of all indexed key names. Never null; empty if no keys
are indexed.
+ Set<String> getKeys();
Review Comment:
For blob storage, does this return all the keys? Or is this only for
materialized columns?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/datasource/MapDataSource.java:
##########
@@ -33,6 +33,19 @@ public interface MapDataSource extends DataSource {
/// Returns the DataSource for the given map key's values.
DataSource getDataSource(String key);
+ /// Returns whether this segment has per-key index data for the given key.
Columnar segments
+ /// return an exact answer (O(1) lookup into the materialized key set).
Blob-only segments
+ /// return {@code true} conservatively because determining key presence
requires deserialization.
Review Comment:
(minor) You may use backticks for code. Same for other places
```suggestion
/// return `true` conservatively because determining key presence requires
deserialization.
```
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/datasource/MapDataSource.java:
##########
@@ -33,6 +33,19 @@ public interface MapDataSource extends DataSource {
/// Returns the DataSource for the given map key's values.
DataSource getDataSource(String key);
+ /// Returns whether this segment has per-key index data for the given key.
Columnar segments
+ /// return an exact answer (O(1) lookup into the materialized key set).
Blob-only segments
+ /// return {@code true} conservatively because determining key presence
requires deserialization.
Review Comment:
Does this mean for blob storage, it always return `true`? I feel it is not
very useful though. What if we just make `getDataSource(String key)` return
`@Nullable`? For blob storage, what is the contract for `getDataSource(String
key)`?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/FieldConfig.java:
##########
@@ -71,6 +71,24 @@ public class FieldConfig extends BaseJsonConfig {
public static final String
TEXT_INDEX_LUCENE_NRT_CACHING_DIRECTORY_BUFFER_SIZE =
"luceneNRTCachingDirectoryMaxBufferSizeMB";
+ /// MAP index property keys, passed via `FieldConfig.properties`.
+ /// See `MapIndexConfig` for semantics.
+
+ /// Maximum number of MAP keys to materialise as dense columns (default
1000).
+ public static final String MAP_INDEX_MAX_DENSE_KEYS = "maxDenseKeys";
Review Comment:
Do we want to limit the total dense keys? I feel all keys meeting the min
fill rate should be materialized. If not, how do you choose the keys to
materialize?
--
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]
