Jackie-Jiang commented on code in PR #9413:
URL: https://github.com/apache/pinot/pull/9413#discussion_r976967249
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/SegmentMetadata.java:
##########
@@ -53,6 +53,10 @@ public interface SegmentMetadata {
long getEndTime();
+ String getRawStartTime();
Review Comment:
Similarly we don't need these 2 methods. We can use the min/max value from
the time column metadata
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -746,6 +748,9 @@ private void writeMetadata()
} else {
// No records in segment. Use current time as start/end
long now = System.currentTimeMillis();
+ properties.setProperty(RAW_SEGMENT_START_TIME,
String.valueOf(now));
Review Comment:
This is incorrect. We shouldn't set them here because millis value doesn't
necessarily match the time format
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -727,6 +727,8 @@ private void writeMetadata()
if (_totalDocs > 0) {
String startTimeStr =
timeColumnIndexCreationInfo.getMin().toString();
String endTimeStr =
timeColumnIndexCreationInfo.getMax().toString();
+ properties.setProperty(RAW_SEGMENT_START_TIME, startTimeStr);
Review Comment:
We don't need to persist them as new fields because they are just the
min/max value for the time column.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java:
##########
@@ -688,13 +688,18 @@ public void updateBooleanFieldsIfNeeded(Schema oldSchema)
{
}
}
+
+ public boolean isBackwardCompatibleWith(Schema oldSchema) {
+ return isBackwardCompatibleWith(oldSchema, false);
+ }
+
/**
* Check whether the current schema is backward compatible with oldSchema.
* Backward compatibility requires all columns and fieldSpec in oldSchema
should be retained.
*
* @param oldSchema old schema
*/
- public boolean isBackwardCompatibleWith(Schema oldSchema) {
+ public boolean isBackwardCompatibleWith(Schema oldSchema, boolean
skipTimeColumn) {
Review Comment:
Can you please rebase to the latest master? We already fixed this and allow
time format change for time
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/util/ZKMetadataUtils.java:
##########
@@ -56,6 +60,27 @@ public static void refreshSegmentZKMetadata(String
tableNameWithType, SegmentZKM
segmentSizeInBytes, false);
}
+ public static void updateSegmentZKMetadataInterval(TableConfig tableConfig,
SegmentZKMetadata segmentZKMetadata,
+ Schema newSchema) {
+ String timeColumnName =
tableConfig.getValidationConfig().getTimeColumnName();
+ if (StringUtils.isNotEmpty(timeColumnName)) {
+ DateTimeFieldSpec newDateTimeFieldSpec =
newSchema.getDateTimeSpec(timeColumnName);
+
+ assert newDateTimeFieldSpec != null;
+ String startTimeString = segmentZKMetadata.getRawStartTime();
+ if (StringUtils.isNotEmpty(startTimeString)) {
+ long updatedStartTime =
newDateTimeFieldSpec.getFormatSpec().fromFormatToMillis(startTimeString);
+ segmentZKMetadata.setStartTime(updatedStartTime);
Review Comment:
We need to convert the start/end time based on the time unit
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -2254,6 +2277,16 @@ private void deleteTableOnServer(String
tableNameWithType) {
}
}
+ public void updateSegmentInterval(String tableNameWithType,
SegmentZKMetadata segmentZKMetadata,
+ Schema newSchema) {
+ String segmentName = segmentZKMetadata.getSegmentName();
+ TableConfig tableConfig = getTableConfig(tableNameWithType);
Review Comment:
We don't want to read table config for each segment. We should read the
table config once, and pass it into this method
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -2254,6 +2277,16 @@ private void deleteTableOnServer(String
tableNameWithType) {
}
}
+ public void updateSegmentInterval(String tableNameWithType,
SegmentZKMetadata segmentZKMetadata,
+ Schema newSchema) {
+ String segmentName = segmentZKMetadata.getSegmentName();
+ TableConfig tableConfig = getTableConfig(tableNameWithType);
+ if (tableConfig != null) {
+ ZKMetadataUtils.updateSegmentZKMetadataInterval(tableConfig,
segmentZKMetadata, newSchema);
Review Comment:
Throw exception when table config is not available
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -200,6 +200,34 @@ public ConfigSuccessResponse updateSchema(
return new ConfigSuccessResponse(successResponse.getStatus(),
schemaAndUnrecognizedProps.getRight());
}
+ @PUT
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @Path("/schemas/fixDateTimeFormat/{schemaName}")
+ @Authenticate(AccessType.UPDATE)
+ @ApiOperation(value = "Update date time format in schema", notes = "Update
date time format in schema")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Successfully updated schema"),
+ @ApiResponse(code = 404, message = "Schema not found"),
+ @ApiResponse(code = 400, message = "Missing or invalid request body"),
+ @ApiResponse(code = 500, message = "Internal error")
+ })
+ public ConfigSuccessResponse fixDateTimeFormat(
+ @ApiParam(value = "Name of the schema", required = true)
@PathParam("schemaName") String schemaName,
+ @ApiParam(value = "Whether to refresh the segments after updating
metadata") @DefaultValue("false")
+ @QueryParam("refresh") boolean refresh, String schemaJsonString) {
Review Comment:
We shouldn't need to refresh the segments
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1278,8 +1278,31 @@ public void addSchema(Schema schema, boolean override)
}
}
+ public void updateSchemaDateTime(Schema schema, boolean refresh)
Review Comment:
I don't think refresh matters here
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -200,6 +200,34 @@ public ConfigSuccessResponse updateSchema(
return new ConfigSuccessResponse(successResponse.getStatus(),
schemaAndUnrecognizedProps.getRight());
}
+ @PUT
+ @Produces(MediaType.APPLICATION_JSON)
+ @Consumes(MediaType.APPLICATION_JSON)
+ @Path("/schemas/fixDateTimeFormat/{schemaName}")
+ @Authenticate(AccessType.UPDATE)
+ @ApiOperation(value = "Update date time format in schema", notes = "Update
date time format in schema")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Successfully updated schema"),
+ @ApiResponse(code = 404, message = "Schema not found"),
+ @ApiResponse(code = 400, message = "Missing or invalid request body"),
+ @ApiResponse(code = 500, message = "Internal error")
+ })
+ public ConfigSuccessResponse fixDateTimeFormat(
+ @ApiParam(value = "Name of the schema", required = true)
@PathParam("schemaName") String schemaName,
Review Comment:
We want to take the table name instead of the schema name. We can put it
under the `PinotSegmentRestletResource`, and read the table config and schema
for the table from the cluster.
--
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]
