Jackie-Jiang commented on code in PR #8343:
URL: https://github.com/apache/pinot/pull/8343#discussion_r845659983
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/SegmentGeneratorConfig.java:
##########
@@ -202,6 +207,33 @@ public SegmentGeneratorConfig(TableConfig tableConfig,
Schema schema) {
}
}
+ public static Schema updateSchemaWithTimestampIndexes(Schema schema,
+ Map<String, List<TimestampIndexGranularity>> timestampIndexConfigs) {
+ if (timestampIndexConfigs.isEmpty()) {
+ return schema;
+ }
+ List<FieldSpec> timestampColumnWithGranularityFieldSpecs = new
ArrayList<>();
+ for (String columnName : timestampIndexConfigs.keySet()) {
Review Comment:
(minor) Use `.entrySet()` to avoid extra lookup
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java:
##########
@@ -156,7 +164,8 @@ public static ImmutableSegment load(SegmentDirectory
segmentDirectory, IndexLoad
Map<String, ColumnMetadata> columnMetadataMap =
segmentMetadata.getColumnMetadataMap();
if (schema != null) {
Set<String> columnsInMetadata = new
HashSet<>(columnMetadataMap.keySet());
- columnsInMetadata.removeIf(schema::hasColumn);
+ Set<String> extraIndexedColumns =
extractIndexedColumns(indexLoadingConfig.getTableConfig());
Review Comment:
Do we still need this since the column is already added to the schema?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/SegmentGeneratorConfig.java:
##########
@@ -202,6 +207,33 @@ public SegmentGeneratorConfig(TableConfig tableConfig,
Schema schema) {
}
}
+ public static Schema updateSchemaWithTimestampIndexes(Schema schema,
+ Map<String, List<TimestampIndexGranularity>> timestampIndexConfigs) {
+ if (timestampIndexConfigs.isEmpty()) {
+ return schema;
+ }
+ List<FieldSpec> timestampColumnWithGranularityFieldSpecs = new
ArrayList<>();
+ for (String columnName : timestampIndexConfigs.keySet()) {
+ Preconditions.checkState(schema.hasColumn(columnName),
+ "Cannot create Timestamp index for column: %s because it is not in
schema", columnName);
+ timestampIndexConfigs.get(columnName).stream().filter(granularity ->
!schema.hasColumn(
+ TimestampIndexGranularity.getColumnNameWithGranularity(columnName,
granularity))).forEach(
+ granularity -> timestampColumnWithGranularityFieldSpecs.add(
+
TimestampIndexGranularity.getFieldSpecForTimestampColumnWithGranularity(
+ schema.getFieldSpecFor(columnName), granularity)));
+ }
+ if (timestampColumnWithGranularityFieldSpecs.isEmpty()) {
+ return schema;
+ }
+ Schema newSchema = new Schema.SchemaBuilder()
Review Comment:
Why do we create a new schema? Per the method name it should update the
schema in-place. Simply do `schema.addField()` should do the work
##########
pinot-common/src/main/java/org/apache/pinot/common/config/provider/TableCache.java:
##########
@@ -89,6 +90,8 @@
private final ZkSchemaChangeListener _zkSchemaChangeListener = new
ZkSchemaChangeListener();
// Key is schema name, value is schema info
private final Map<String, SchemaInfo> _schemaInfoMap = new
ConcurrentHashMap<>();
+ // Key is table name with type, value is all the timestamp with granularity
column names
+ private final Map<String, Set<String>> _timestampIndexColumnsMap = new
ConcurrentHashMap<>();
Review Comment:
Don't add the extra map, add the set into the `TableConfigInfo` (similar to
`_expressionOverrideMap`)
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/SegmentGeneratorConfig.java:
##########
@@ -261,6 +293,22 @@ private void
extractH3IndexConfigsFromTableConfig(TableConfig tableConfig) {
}
}
+ public static Map<String, List<TimestampIndexGranularity>>
extractTimestampIndexConfigsFromTableConfig(
+ TableConfig tableConfig) {
+ if (tableConfig == null) {
+ return ImmutableMap.of();
Review Comment:
(minor)
```suggestion
return Collections.emptyMap();
```
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
