xiangfu0 commented on code in PR #18639:
URL: https://github.com/apache/pinot/pull/18639#discussion_r3330194056
##########
pinot-sql-ddl/src/main/java/org/apache/pinot/sql/ddl/compile/DdlCompiler.java:
##########
@@ -598,10 +632,17 @@ private static CompiledCreateMaterializedView
compileCreateMaterializedView(Stri
TableConfigBuilder builder = new TableConfigBuilder(TableType.OFFLINE)
.setTableName(tableNameForConfig)
.setIsMaterializedView(true);
- MaterializedViewPropertyRouter.apply(properties, definedSql, schedule,
builder);
+ // The handler routes the MV properties onto the builder and returns the
minion task type it
+ // stamped (default: MaterializedViewTask). The consistency check below
uses that task type.
+ String mvTaskType = mvHandler.applyTaskConfig(properties, definedSql,
schedule, builder);
Review Comment:
Allowing a handler to return an arbitrary task type here looks incomplete.
The OSS runtime still reads MV task config only from
`MaterializedViewTask.TASK_TYPE` in places like
`MaterializedViewDefinitionMetadataBuilder` and `PinotHelixResourceManager`, so
a custom handler can create a table that later readers treat as missing
`definedSql` / `bucketTimePeriod`. This needs the downstream readers
generalized in the same change, otherwise the new extension point is not
actually safe to use.
--
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]
