feat(columnar_map): SPI and data model for COLUMNAR_MAP index #18368
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,9 +32,22 @@ public interface MapDataSource extends DataSource { | |||||||
|
|
||||||||
| /** | ||||||||
| * Get the Data Source representation of a single key within this map column. | ||||||||
| * Only call after confirming the key exists via {@link #containsKey(String)}. | ||||||||
| */ | ||||||||
| DataSource getKeyDataSource(String key); | ||||||||
|
Contributor
There was a problem hiding this comment. Not introduced in this PR, but let's rename it to:
Suggested change
It is very confusing now because the data source is for value, not key.
Contributor
Author
There was a problem hiding this comment. Agreed on the naming — getDataSource(key) is cleaner. I'll do the full getKeyXXX → getXXX rename across the SPI in a separate refactoring PR so it's one clean sweep rather than scattered across the stack. Will raise a new PR for this.
Contributor
Author
There was a problem hiding this comment. For @nullable: I'd prefer to keep the current non-null contract backed by NullDataSource. The callers in ProjectionBlock and ItemTransformFunction dereference the result immediately without a null-check — NullDataSource lets them do that safely by returning the column's default value for every doc. If we switch to @nullable, those two callers need null-guards, and so does any future caller. NullDataSource gives the same semantic (absent key → null/default for all rows) without pushing null-handling into every call site. Let me know your thoughts. |
||||||||
|
|
||||||||
| /** | ||||||||
| * Returns true if {@code key} is present in this MAP column for at least one document in | ||||||||
| * this segment. Call this before {@link #getKeyDataSource(String)} to avoid undefined | ||||||||
| * behaviour on absent keys. | ||||||||
|
Collaborator
There was a problem hiding this comment. Should not absent keys fall into Sparse Key reader Path? Do we need this ?
Contributor
Author
There was a problem hiding this comment. ContainsKey is currently used as an early-exit guard in MapFilterOperator and the aggregation/group-by plan nodes to skip processing when a key is absent from the segment (backed by the map key set) which is O(1) lookup. Removing it would mean every caller replaces containsKey(key) with getDataSource(key) != null, which triggers a full NullDataSource construction just to check presence. |
||||||||
| * | ||||||||
| * <p>The default implementation delegates to {@link #getKeyDataSources()}, which may be | ||||||||
| * expensive for large key sets. Implementations should override for O(1) performance. | ||||||||
| */ | ||||||||
| default boolean containsKey(String key) { | ||||||||
|
Contributor
There was a problem hiding this comment. (optional) This is probably not needed if we make
Contributor
Author
There was a problem hiding this comment. replied above.
|
||||||||
| return getKeyDataSources().containsKey(key); | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Get the Data Source representation of all keys within this map column. | ||||||||
| */ | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| /** | ||
| * 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.creator; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Collections; | ||
| import java.util.Map; | ||
| import javax.annotation.Nullable; | ||
| import org.apache.commons.configuration2.PropertiesConfiguration; | ||
| import org.apache.pinot.segment.spi.index.IndexCreator; | ||
|
|
||
|
|
||
| /** | ||
|
tarun11Mavani marked this conversation as resolved.
Outdated
|
||
| * Creator for the COLUMNAR_MAP index. Accepts one map per document during segment creation | ||
| * and decomposes it into per-key columnar storage on seal(). | ||
| * | ||
| * <p>Implementations are not thread-safe; callers must serialize {@link #add} calls per | ||
| * creator instance. | ||
| * | ||
| * <p>The inherited {@code add(Object, int)} method from {@link IndexCreator} treats the | ||
| * first argument as the map and the second as the docId, matching the column-major creator | ||
| * path. Callers may use either entry point. | ||
| */ | ||
| public interface ColumnarMapIndexCreator extends IndexCreator { | ||
|
|
||
| /** | ||
| * Adds one document's map. Keys present in the map's entry set are routed to per-key | ||
| * columnar storage; keys with declared types are coerced to those types, others fall | ||
| * back to the configured default value type. A null or empty map is valid and means the | ||
| * document has no key/value pairs. | ||
| * | ||
| * @param mapValue the document's map (may be null or empty) | ||
| * @param docId the document id, must be monotonically non-decreasing across calls | ||
| */ | ||
| void add(@Nullable Map<String, Object> mapValue, int docId) | ||
|
tarun11Mavani marked this conversation as resolved.
Outdated
|
||
| throws IOException; | ||
|
|
||
| /** | ||
| * Returns metadata properties for any virtual columns this creator materialized during | ||
| * {@code seal()}. The framework merges the returned properties into the segment metadata. | ||
| * Implementations that do not produce virtual columns return an empty map. | ||
| * | ||
| * <p>Call after {@code seal()}. | ||
| * | ||
| * @return a map from virtual-column name to its {@link PropertiesConfiguration}; never null | ||
| */ | ||
| default Map<String, PropertiesConfiguration> getVirtualColumnMetadata() { | ||
| return Collections.emptyMap(); | ||
|
tarun11Mavani marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share how does columnMetadata and index_map looks like for virtual columns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure. I have it documented here.
https://docs.google.com/document/d/14kPmjDTKbO8l0ql4rrN7I5Yki5pqMw6GeGmxxc9grsU/edit?tab=t.0#bookmark=id.kycvq78ioe5g