Copilot commented on code in PR #18544:
URL: https://github.com/apache/pinot/pull/18544#discussion_r3303786589
##########
pinot-sql-ddl/src/main/java/org/apache/pinot/sql/ddl/reverse/CanonicalDdlEmitter.java:
##########
@@ -59,30 +78,131 @@ public static String emit(Schema schema, TableConfig
config) {
/// Renders the canonical DDL, scoped under `databaseName` when non-null.
The database name
/// is rendered as a leading `db.` qualifier on the table name; this matches
the parser's
/// `[db.]name` grammar.
+ ///
+ /// Dispatches to [#emitMaterializedView] when `config.isMaterializedView()`
is true; otherwise
+ /// renders a regular `CREATE TABLE`.
public static String emit(Schema schema, TableConfig config, @Nullable
String databaseName) {
+ if (MaterializedViewPropertyExtractor.isMaterializedView(config)) {
+ return emitMaterializedView(schema, config, databaseName);
+ }
+ return emitTable(schema, config, databaseName);
+ }
+
+ private static String emitTable(Schema schema, TableConfig config, @Nullable
String databaseName) {
rejectUnsupportedSchemaMetadata(schema);
StringBuilder sb = new StringBuilder(512);
- String rawTableName =
TableNameBuilder.extractRawTableName(config.getTableName());
-
- // Derive the effective database: explicit argument wins; fall back to any
db. prefix already
- // embedded in the raw table name (e.g. analytics.events_OFFLINE →
"analytics"). This ensures
- // the no-database overload emit(schema, config) preserves a db-qualified
table name rather
- // than silently stripping it to a bare name that would land in the wrong
database on replay.
- String effectiveDb = databaseName;
- if ((effectiveDb == null || effectiveDb.isEmpty()) &&
rawTableName.contains(".")) {
- int dot = rawTableName.indexOf('.');
- effectiveDb = rawTableName.substring(0, dot);
- }
- String displayName = rawTableName.contains(".") ?
rawTableName.substring(rawTableName.indexOf('.') + 1)
- : rawTableName;
+ QualifiedName name = resolveQualifiedName(config, databaseName);
sb.append("CREATE TABLE ");
- if (effectiveDb != null && !effectiveDb.isEmpty()) {
- sb.append(SqlIdentifiers.quote(effectiveDb)).append('.');
+ appendQualifiedName(sb, name);
+ sb.append(" (\n");
+ appendColumnBlock(sb, schema);
+ sb.append(")\n");
+ appendPrimaryKey(sb, schema);
+
+ sb.append("TABLE_TYPE =
").append(emitTableType(config.getTableType())).append('\n');
+
+ Map<String, String> props = PropertyExtractor.extract(config);
+ appendPropertiesAndTerminate(sb, props);
+ return sb.toString();
+ }
+
+ /// Renders `CREATE MATERIALIZED VIEW [db.]name ( ... ) [REFRESH EVERY
'<period>']
+ /// [PROPERTIES (...)] AS <definedSQL>;`.
+ ///
+ /// Failure modes:
+ /// - `task.MaterializedViewTask.definedSQL` missing/empty (defensive —
caller should have
+ /// gated on [#emit]'s [TableConfig#isMaterializedView] check, and the
SPI's
+ /// `TableConfigUtils.validateMaterializedViewInvariants` makes this state
unreachable for
+ /// any persisted config) → IllegalArgumentException so the controller
surfaces a 400 rather
+ /// than emit headerless / bodiless DDL.
+ /// - `task.MaterializedViewTask.schedule` present but
[MaterializedViewPropertyRouter#cronToPeriod]
+ /// returns null (non-standard / hand-typed cron) →
IllegalArgumentException (Q1=A
+ /// contract). The DDL grammar has no syntax for raw cron, so silently
dropping the
+ /// schedule on `SHOW CREATE MATERIALIZED VIEW` would let `emit → parse →
emit` produce a
+ /// config that runs on the cluster-wide cron instead of the
operator-chosen one — a
+ /// user-invisible behavioural change. The controller maps the exception
to 400 with the
+ /// message routed back to the caller.
+ private static String emitMaterializedView(Schema schema, TableConfig config,
+ @Nullable String databaseName) {
+ rejectUnsupportedSchemaMetadata(schema);
+ StringBuilder sb = new StringBuilder(512);
+
+ QualifiedName name = resolveQualifiedName(config, databaseName);
+ Map<String, String> mvTaskConfig = config.getTaskConfig()
+ .getConfigsForTaskType(MaterializedViewTask.TASK_TYPE);
+ String definedSql = mvTaskConfig.get(MaterializedViewTask.DEFINED_SQL_KEY);
Review Comment:
`emitMaterializedView()` dereferences `config.getTaskConfig()` without a
null check. If a TableConfig has `isMaterializedView=true` but its task config
is missing/corrupted (or not yet validated), this will NPE before the intended
`definedSQL` guard and will likely surface as a 500 instead of an actionable
400. Consider defensively checking `config.getTaskConfig()` (and the returned
`mvTaskConfig` map) for null and throwing an `IllegalArgumentException` with a
clear message when missing.
##########
pinot-sql-ddl/src/main/java/org/apache/pinot/sql/ddl/reverse/SqlIdentifiers.java:
##########
@@ -36,6 +36,8 @@ final class SqlIdentifiers {
// grammar keywords introduced by Pinot DDL that could appear as user
identifiers
"DIMENSION", "METRIC", "DATETIME", "FORMAT", "GRANULARITY", "OFFLINE",
"REALTIME",
"PROPERTIES", "TABLES", "TABLE_TYPE", "IF", "TYPE", "FROM",
+ // additional keywords introduced by the CREATE / SHOW CREATE
MATERIALIZED VIEW grammar
+ "MATERIALIZED", "VIEW", "REFRESH", "EVERY",
// standard SQL keywords that round-trip incorrectly when bare
Review Comment:
The quoting allow-list adds MV keywords but omits the plural token `VIEWS`
(used by `SHOW MATERIALIZED VIEWS`). For consistency with other Pinot-DDL
keywords in `ALWAYS_QUOTE`, and to avoid emitting ambiguous/unquoted
identifiers named `views`, add `"VIEWS"` to `ALWAYS_QUOTE`.
##########
pinot-materialized-view/src/test/java/org/apache/pinot/materializedview/metadata/MaterializedViewMetadataTest.java:
##########
@@ -154,4 +155,89 @@ public void testWatermarkAlwaysWritten() {
assertTrue(znRecord.getSimpleFields().containsKey("watermarkMs"),
"watermarkMs key must always be written");
}
+
+ /// Definition equality is the controller's idempotency check at CREATE MV
time: a second
+ /// CREATE with the same definedSQL must hash/compare equal to the first so
a stale orphan
+ /// znode from a prior failed CREATE doesn't get classified as "different
content".
+ /// Verify every persisted field round-trips through equals/hashCode.
+ @Test
Review Comment:
The comment says definition equality/hashCode covers “every persisted
field”, but the very next test notes `MaterializedViewSplitSpec` intentionally
excludes `materializedViewTimeFormat` from equality even though it is persisted
in the ZNRecord. Please tighten the wording here to avoid implying a stronger
contract than the implementation provides.
--
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]
