Copilot commented on code in PR #14139:
URL: https://github.com/apache/gravitino/pull/14139#discussion_r4345407142
##########
scripts/postgresql/schema-2.0.0-postgresql.sql:
##########
@@ -1036,7 +1036,11 @@ CREATE TABLE IF NOT EXISTS semantic_model_version_info (
version INT NOT NULL,
semantic_model_name VARCHAR(128) NOT NULL,
semantic_model_comment TEXT DEFAULT NULL,
- semantic_model_definition TEXT NOT NULL,
+ datasets TEXT NOT NULL,
+ relationships TEXT DEFAULT NULL,
+ metrics TEXT DEFAULT NULL,
+ ai_context TEXT DEFAULT NULL,
+ custom_extensions TEXT DEFAULT NULL,
properties TEXT DEFAULT NULL,
audit_info TEXT NOT NULL,
Review Comment:
Optional: since these columns are explicitly JSON snapshots and the stated
goal is to enable more targeted reads (potentially including server-side
inspection/filtering), consider using PostgreSQL `jsonb` (and MySQL `JSON`
where available) instead of `TEXT` to enable JSON validation and more efficient
JSON operators/indexing. Tradeoff: cross-database portability and migration
complexity vs. improved queryability and reduced risk of persisting non-JSON
content.
##########
core/src/main/java/org/apache/gravitino/storage/relational/po/SemanticModelPO.java:
##########
@@ -111,8 +117,17 @@ public static SemanticModelEntity fromSemanticModelPO(
try {
SemanticModelVersionInfoPO versionPO =
semanticModelPO.getSemanticModelVersionInfoPO();
SemanticModelDefinitionDTO definitionDTO =
- JsonUtils.anyFieldMapper()
- .readValue(versionPO.semanticModelDefinition(),
SemanticModelDefinitionDTO.class);
+ SemanticModelDefinitionDTO.builder()
+ .withDatasets(
+ JsonUtils.anyFieldMapper().readValue(versionPO.datasets(),
DatasetDTO[].class))
+ .withRelationships(
+ deserializeOptionalField(versionPO.relationships(),
RelationshipDTO[].class))
+ .withMetrics(deserializeOptionalField(versionPO.metrics(),
MetricDTO[].class))
+ .withAiContext(deserializeOptionalField(versionPO.aiContext(),
AIContextDTO.class))
+ .withCustomExtensions(
+ deserializeOptionalField(
+ versionPO.customExtensions(),
CustomExtensionDTO[].class))
+ .build();
Review Comment:
With the definition split into multiple JSON columns, a deserialization
failure will surface as a generic `JsonProcessingException` without indicating
which column was malformed. Consider deserializing each field separately and
wrapping failures with field context (e.g., include the column/field name like
`datasets` / `metrics` / `ai_context`) so operational debugging can pinpoint
the corrupted column quickly.
##########
core/src/main/java/org/apache/gravitino/storage/relational/po/SemanticModelVersionInfoPO.java:
##########
@@ -70,8 +79,7 @@ private SemanticModelVersionInfoPO(
Preconditions.checkArgument(
StringUtils.isNotBlank(semanticModelName), "Semantic Model name cannot
be empty");
Preconditions.checkArgument(
- StringUtils.isNotBlank(semanticModelDefinition),
- "Semantic Model definition cannot be empty");
+ StringUtils.isNotBlank(datasets), "Semantic Model datasets cannot be
empty");
Preconditions.checkArgument(StringUtils.isNotBlank(auditInfo), "Audit info
cannot be empty");
Review Comment:
The constructor validates `datasets` as non-blank, but the optional JSON
columns (`relationships`, `metrics`, `aiContext`, `customExtensions`) can still
be persisted as an empty string (non-null but blank), which will later fail
JSON parsing on reads. To prevent storing an invalid intermediate state, add
validation that optional fields are either `null` or `isNotBlank(...)` (or
normalize blank strings to `null` before persistence).
--
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]
