Copilot commented on code in PR #18564:
URL: https://github.com/apache/pinot/pull/18564#discussion_r3286691978
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -1424,6 +1426,73 @@ private static void
validateDedupConfigUpdate(TableConfig newConfig, TableConfig
}
}
+ /**
+ * Validates materialized-view table identity and task-config consistency.
+ * Identity is declared only via {@link TableConfig#isMaterializedView()};
task configs alone do not
+ * make a table an MV.
+ */
+ @VisibleForTesting
+ static void validateMaterializedViewInvariants(TableConfig tableConfig) {
+ boolean isMaterializedView = tableConfig.isMaterializedView();
+ boolean hasMvTaskWithDefinedSql =
tableConfig.hasMaterializedViewTaskWithDefinedSql();
+ boolean hasMvTask = tableConfig.getMaterializedViewTaskConfigs() != null;
+
+ if (isMaterializedView) {
+ Preconditions.checkState(tableConfig.getTableType() == TableType.OFFLINE,
+ "Materialized view tables must be OFFLINE, got: %s for table: %s",
tableConfig.getTableType(),
+ tableConfig.getTableName());
+ Preconditions.checkState(hasMvTaskWithDefinedSql,
+ "isMaterializedView is true but MaterializedViewTask with non-empty
definedSQL is required for table: %s",
+ tableConfig.getTableName());
+ Preconditions.checkState(!tableConfig.isDimTable(),
+ "A table cannot be both isDimTable and isMaterializedView: %s",
tableConfig.getTableName());
+ }
+
+ if (hasMvTaskWithDefinedSql && !isMaterializedView) {
+ throw new IllegalStateException(String.format(
+ "MaterializedViewTask is configured but isMaterializedView is not
true for table: %s. "
+ + "Set \"isMaterializedView\": true or remove
MaterializedViewTask.",
+ tableConfig.getTableName()));
+ }
+
+ if (hasMvTask && !hasMvTaskWithDefinedSql) {
+ throw new IllegalStateException(String.format(
+ "MaterializedViewTask is configured but definedSQL is missing or
empty for table: %s",
+ tableConfig.getTableName()));
+ }
+ }
+
+ private static void validateMaterializedViewConfigUpdate(TableConfig
newConfig, TableConfig existingConfig,
+ List<String> violations) {
+ if (existingConfig.isMaterializedView() != newConfig.isMaterializedView())
{
+ violations.add(String.format("isMaterializedView (%s -> %s) cannot be
changed; drop and recreate the table",
+ existingConfig.isMaterializedView(),
newConfig.isMaterializedView()));
+ }
+
+ if (!existingConfig.isMaterializedView() &&
!newConfig.isMaterializedView()) {
+ return;
+ }
Review Comment:
`validateMaterializedViewConfigUpdate` currently rejects any change to
`isMaterializedView`. This blocks upgrades of existing MV tables created before
this flag existed (they will deserialize with `isMaterializedView=false`), and
prevents users from adding the flag via a PUT even when the table is already an
MV by legacy identity (e.g., has `MaterializedViewTask.definedSQL` / definition
metadata). Consider allowing a one-time "promotion" from false->true when the
existing config already has an MV task with non-empty `definedSQL` (or an MV
definition exists), while still rejecting true->false and other toggles.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -5407,33 +5409,19 @@ private void
notifyMaterializedViewConsistencyManagerForTableDrop(String tableNa
return;
}
try {
+ TableConfig tableConfig =
ZKMetadataProvider.getTableConfig(_propertyStore, tableNameWithType);
+ if (tableConfig != null && !tableConfig.isMaterializedView()) {
+ return;
+ }
MaterializedViewDefinitionMetadata materializedViewDefinition =
MaterializedViewDefinitionMetadataUtils.fetch(_propertyStore,
tableNameWithType);
if (materializedViewDefinition != null &&
materializedViewDefinition.getBaseTables() != null
&& !materializedViewDefinition.getBaseTables().isEmpty()) {
mgr.onMaterializedViewTableDropped(tableNameWithType,
materializedViewDefinition.getBaseTables());
- return;
- }
- // Fall back to reading source table from table task config
- TableConfig tableConfig =
ZKMetadataProvider.getTableConfig(_propertyStore, tableNameWithType);
- if (tableConfig == null) {
- return;
- }
- TableTaskConfig taskConfig = tableConfig.getTaskConfig();
- if (taskConfig == null) {
- return;
- }
- Map<String, String> materializedViewTaskConfigs =
-
taskConfig.getConfigsForTaskType(CommonConstants.MaterializedViewTask.TASK_TYPE);
- if (materializedViewTaskConfigs == null) {
- return;
- }
- String definedSQL =
materializedViewTaskConfigs.get(CommonConstants.MaterializedViewTask.DEFINED_SQL_KEY);
- if (definedSQL == null || definedSQL.isEmpty()) {
- return;
+ } else if (tableConfig != null && tableConfig.isMaterializedView()) {
+ LOGGER.warn("MV table {} dropped without definition metadata;
consistency reverse index may be stale",
+ tableNameWithType);
}
Review Comment:
In the MV drop path, when the definition znode is missing you only log a
warning and do not call `mgr.onMaterializedViewTableDropped(...)`. This can
leave the consistency manager reverse index with a dangling MV entry (e.g.,
create+drop before the scheduler’s first run), causing unnecessary/stale
consistency work on future base-table segment updates. Consider falling back to
extracting base tables from `MaterializedViewTask.definedSQL` (similar to the
create path) and unregistering, even if definition metadata is absent.
--
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]
