[#12210] docs: Add Semantic Model design - #12424
Conversation
Code Coverage Report
|
laserninja
left a comment
laserninja
left a comment
There was a problem hiding this comment.
Checked the contract against the pinned Ossie schema at 88e0011: field names, required/optional markers, cardinalities, and the Dialect/DataType enums all match. Making this a first-class entity also resolves the View-namespace and per-entity-version concerns from the earlier design. Six points below, mostly where the Gravitino side of the contract is still unspecified.
Two nits: the Java example writes DataType.DECIMAL while the wire value is Decimal, and it'd help to say whether DataType relates to org.apache.gravitino.rel.types.Type or deliberately doesn't. Also REST create inlines datasets/relationships/metrics at top level while replaceDefinition nests them under definition - Java uses SemanticModelDefinition for both.
| vendor_name: string | ||
| data: string | ||
|
|
||
| Dialect = "ANSI_SQL" | "SNOWFLAKE" | "MDX" | "TABLEAU" |
There was a problem hiding this comment.
None of Gravitino's own engine dialects appear here. Dialects (api/src/main/java/org/apache/gravitino/rel/Dialects.java:31-40) defines trino, spark, hive, flink and explicitly allows unlisted strings; this enum is closed and has none of them. Worth stating what a user with a Trino- or Spark-specific metric expression does - ANSI_SQL looks like the only option - and whether Gravitino would ever accept a dialect outside the enum, since that breaks the conformance claim below.
| caller cannot access. | ||
|
|
||
| Invalid definitions return `400`. Missing sources or source columns are invalid definitions. A | ||
| temporarily unavailable source catalog returns `503` so the caller can retry. Authorization failures |
There was a problem hiding this comment.
Gravitino has no 503 in its error model. ErrorConstants has no service-unavailable code, and an unreachable underlying catalog already maps to 502 via Utils.connectionFailed (server-common/src/main/java/org/apache/gravitino/server/web/Utils.java:85) with CONNECTION_FAILED_CODE = 1007, which the Java client switches on in ErrorHandlers. Either reuse connection-failed or note that a new code plus client handling is part of this work.
| Dimension | ||
| is_time?: boolean | ||
|
|
||
| AIContext = string | AIContextObject |
There was a problem hiding this comment.
The typed API shape for this union isn't specified - the Java example never constructs an AIContext. The schema sets additionalProperties: true on the object form and the doc promises lossless retention, so it'd help to say whether this is a sealed type with string and object variants and where the unknown properties live (e.g. Map<String, Object>), since that also determines equality and JSON round-tripping.
| - Create writes the identity row and version 1 snapshot in one transaction. | ||
| - Alter writes a complete new snapshot, increments `last_version`, and advances `current_version` in | ||
| the same transaction. Rename also updates `semantic_model_meta.semantic_model_name`; the version | ||
| snapshot retains the name at the time of the change. |
There was a problem hiding this comment.
Nothing here prevents a lost update: two concurrent replaceDefinition calls each validate against the state they read, and the second silently overwrites the first with no way for the caller to detect it. Worth stating the intent - last-write-wins, or an optimistic check against current_version. ErrorConstants.OPTIMISTIC_LOCK_CONFLICT_CODE (1012) already exists if you want the latter.
| ``` | ||
|
|
||
| The projection maps the entity name to `SemanticModel.name`, `comment` to `description`, and each | ||
| source `NameIdentifier` to its three-part string. The request path supplies the metalake and is not |
There was a problem hiding this comment.
The three-part string is ambiguous in both directions. NameIdentifier.parse splits on . with no quoting or escaping (api/src/main/java/org/apache/gravitino/NameIdentifier.java:36), so a catalog, schema, or table name containing a dot can't be parsed back from an imported Ossie source, and export produces a string that doesn't round-trip. Worth stating the escaping rule, or that such names are rejected at write time.
|
|
||
| ### API and Lifecycle | ||
|
|
||
| `Catalog` exposes `asSemanticModelCatalog()`. Because definitions are stored by Gravitino, support |
There was a problem hiding this comment.
Which Catalog.Type values support this? The sibling asTableCatalog/asViewCatalog accessors (api/src/main/java/org/apache/gravitino/Catalog.java:214-254) throw UnsupportedOperationException per catalog type, and "every catalog that supports schemas" in the Connector section would include FILESET, MESSAGING, and MODEL catalogs, whose schemas contain no Tables or Views to reference. If the intent is RELATIONAL only, say so.
There was a problem hiding this comment.
Only supports the relational catalog.
| KEY `idx_smvi_sid` (`schema_id`) | ||
| ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin | ||
| COMMENT 'semantic model version information'; | ||
| ``` |
There was a problem hiding this comment.
We should support OCC mechanism for CRUD like what @yuqi1129 did for now, just FYI.
| | Privilege | Purpose | | ||
| | ------------------------- | ------------------------------------------------- | | ||
| | `CREATE_SEMANTIC_MODEL` | Create a Semantic Model under a schema | | ||
| | `USE_SEMANTIC_MODEL` | Discover and load the model definition | |
There was a problem hiding this comment.
Shall we align with the table definition like "SELECT_SEMANTIC_MODEL"?
3 participants
What changes were proposed in this pull request?
Add a design document that models an Ossie-compatible
SemanticModelas an independent,first-class, schema-scoped metadata object in Gravitino. The document defines its object model,
validation, APIs, lifecycle, persistence, governance, connector behavior, UI support, and
development plan.
Why are the changes needed?
Semantic definitions describe datasets, fields, relationships, and metrics that consumers combine
at query time. They are not inherently relational Views with fixed query logic and output schemas.
A dedicated metadata object provides clear identity, lifecycle, and governance without overloading
View semantics.
Fix: #12210
Does this PR introduce any user-facing change?
No. This is a design-document-only change; the proposed APIs and behavior are not implemented yet.
How was this patch tested?
Not applicable. This is a design-document-only change.