hongkunxu commented on code in PR #18544:
URL: https://github.com/apache/pinot/pull/18544#discussion_r3308183037
##########
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:
fixed
##########
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:
fixed
--
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]
