Copilot commented on code in PR #17789:
URL: https://github.com/apache/pinot/pull/17789#discussion_r2901313138
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -949,6 +963,8 @@ static void validateTTLForUpsertConfig(TableConfig
tableConfig, Schema schema) {
comparisonColumn, comparisonColumnDataType);
} else {
String comparisonColumn =
tableConfig.getValidationConfig().getTimeColumnName();
+ Preconditions.checkState(comparisonColumn != null,
+ "MetadataTTL / DeletedKeysTTL requires either a comparison column or
a time column to be configured");
DataType comparisonColumnDataType =
schema.getFieldSpecFor(comparisonColumn).getDataType();
Preconditions.checkState(isValidTimeComparisonType(comparisonColumnDataType),
Review Comment:
`schema.getFieldSpecFor(comparisonColumn)` can return null if the configured
time column is missing from the schema, which would cause an NPE when calling
`.getDataType()`. Please add an explicit `schema.hasColumn(comparisonColumn)` /
null check with a clear validation error before dereferencing the FieldSpec.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -784,9 +791,135 @@ public List<SegmentContext>
getSegmentContexts(List<IndexSegment> selectedSegmen
Map<String, String> queryOptions) {
List<SegmentContext> segmentContexts = new
ArrayList<>(selectedSegments.size());
selectedSegments.forEach(s -> segmentContexts.add(new SegmentContext(s)));
+ if (isUpsertEnabled() && !QueryOptionsUtils.isSkipUpsert(queryOptions)) {
+ _tableUpsertMetadataManager.setSegmentContexts(segmentContexts,
queryOptions);
+ }
return segmentContexts;
}
+ @Override
+ public boolean isUpsertEnabled() {
+ return _tableUpsertMetadataManager != null;
+ }
+
+ @VisibleForTesting
+ @Override
+ public TableUpsertMetadataManager getTableUpsertMetadataManager() {
+ return _tableUpsertMetadataManager;
+ }
+
+ @Override
+ public Map<Integer, Long> getPartitionToPrimaryKeyCount() {
+ if (isUpsertEnabled()) {
+ return _tableUpsertMetadataManager.getPartitionToPrimaryKeyCount();
+ }
+ return Collections.emptyMap();
+ }
+
+ protected void handleUpsert(ImmutableSegment immutableSegment, @Nullable
SegmentZKMetadata zkMetadata) {
+ String segmentName = immutableSegment.getSegmentName();
+ _logger.info("Adding immutable segment: {} with upsert enabled",
segmentName);
+ Integer partitionId;
+ if (zkMetadata == null &&
TableNameBuilder.isOfflineTableResource(_tableNameWithType)) {
+ zkMetadata =
ZKMetadataProvider.getSegmentZKMetadata(_helixManager.getHelixPropertyStore(),
_tableNameWithType,
+ segmentName);
+ }
+ setZkOperationTimeIfAvailable(immutableSegment, zkMetadata);
+ if (TableNameBuilder.isOfflineTableResource(_tableNameWithType)) {
+ Preconditions.checkState(zkMetadata != null,
+ "Failed to find segment ZK metadata for segment: %s of OFFLINE
table: %s", segmentName, _tableNameWithType);
+ partitionId = SegmentUtils.getSegmentPartitionId(zkMetadata, null);
+ } else {
Review Comment:
For OFFLINE tables, partition id is derived via
`SegmentUtils.getSegmentPartitionId(zkMetadata, null)`. Passing a null
partition column only works when segment partition metadata contains exactly
one partition column; otherwise this can return null and fail the upsert path.
Consider passing the configured partition column (e.g., from
`segmentsConfig.replicaGroupStrategyConfig.partitionColumn` or the single entry
in `segmentPartitionConfig.columnPartitionMap`) into
`getSegmentPartitionId(...)` so partition id resolution is robust.
--
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]
