feat: return metadata in the SL API - #2505
Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
1a7e688 to
f7d2443
Compare
f7d2443 to
4f23be8
Compare
430d217 to
3ae970a
Compare
|
@betodealmeida can you clarify what you mean by consolidate semantic APIs? Do you mean the various semantic metadata APIs that we provide (e.g., under |
| if revision.type == NodeType.METRIC: | ||
| if revision.name != column_id: | ||
| continue | ||
| raw = revision.custom_metadata or {} |
There was a problem hiding this comment.
Hmm, so looking at this part, when a node's custom_metadata has no semantic_layer key, raw becomes the entirety of custom_metadata right? Is that intentional? I thought this would only return the dict under the semantic_layer key, since all other items on the the custom_metadata could be completely unrelated.
| "percentage", | ||
| "proportion", | ||
| "count", | ||
| "duration", |
There was a problem hiding this comment.
minor: It looks like "duration" is not a member of UnitKind
| } | ||
| elif format_is_explicit and preset == "percentage": | ||
| d3format = f".{precision}%" | ||
| decimals = f".{''.join('0' for _ in range(precision))}" if precision else "" |
There was a problem hiding this comment.
Could this part just reuse _fixed_decimal_pattern rather than inlining a similar expression?
| }, | ||
| }, | ||
| }, | ||
| "owner": "finance", |
There was a problem hiding this comment.
If I'm reading this setup correctly, this can't actually deploy successfully due to the owner field which isn't part of the semantic layer JSON schema. I wonder if you could add a test that actually asserts that failure?
| if isinstance(raw_metadata.get("extensions"), Mapping) | ||
| else {} | ||
| ) | ||
| producer_metadata = { |
There was a problem hiding this comment.
Does this mean that if a node has a semantic_layer key under custom_metadata, e.g.:
custom_metadata:
semantic_layer:
format: {preset: currency}
owner: finance # a random keyThen raw_metadata becomes {format: ..., owner: finance}, which would fail the JSON schema validation.
However, if a node doesn't have the semantic_layer key, e.g.:
custom_metadata:
tier: 2
something: "abcd"Then _raw_column_metadata doesn't find the key, so raw_metadata just becomes everything in custom_metadata. I assume that's unintentional?
Yeah, sorry if I wasn't clear. What I mean is that we have the DJ-native API ( |
|
@betodealmeida Yeah, I think for simplicity for the time being we might just want to keep both for now. If we do remove or try to consolidate, we'll need to look at existing consumers of those APIs and make sure they're migrated before we can remove. |
Summary
This PR adds richer metadata to the semantic layer API responses, allowing clients to provide a better UX. For example, this DJ metric:
Is returned as:
{ "id": "finance.total_revenue", "name": "total_revenue", "type": "floating", "definition": "finance.total_revenue", "description": "Total recognized revenue", "aggregation": "OTHER", "metadata": { "display_name": "Total Revenue", "semantic_type": "currency", "unit": { "kind": "currency", "code": "USD" }, "format": { "preset": "currency" }, "filter": { "kind": "number", "operators": [ "=", "!=", ">", ">=", "<", "<=", "IS NULL", "IS NOT NULL" ], "default_operator": "=" }, "extensions": { "superset": { "d3format": "$,.2f" }, "google_sheets": { "numberFormat": { "type": "CURRENCY", "pattern": "$#,##0.00" } } } } }Fields are generated conservatively based on DJ metadata, while allowing for custom overrides:
@shangyian I wonder if it makes sense to eventually consolidate the "semantic" APIs that DJ has?
Test Plan
make checkpassesmake testshows 100% unit test coverageDeployment Plan