Add datatype field to Field and Metric; reframe is_time as role marker#113
Add datatype field to Field and Metric; reframe is_time as role marker#113jonmmease wants to merge 13 commits into
Conversation
| |----------|-------------| | ||
| | `string` | Variable-length Unicode character data. | | ||
| | `integer` | Signed integer with no scale. | | ||
| | `number` | Real number (floating-point or decimal) with unspecified precision. | |
There was a problem hiding this comment.
One concern here is that using a generic number type isn’t really a valid interoperability strategy. Different data warehouses interpret and implement it differently, so it’s not interchangeable in practice.
For example, you can’t take a model built against data in Snowflake and expect it to behave identically in Databricks just because both use a number type. As we’ve already discussed in other issues/threads, precision and scale vary across systems, and that directly affects results.
Without explicitly specifying those parameters, this approach is likely to introduce subtle bugs. If strict type checking is enforced and there’s no automatic conversion, things will simply break. Even with implicit or explicit casting at the physical layer, you can still end up with precision loss, which is not acceptable for many use cases.
So I think we need a different approach here. Either we make the type definition explicit (e.g., always specifying precision and scale for numeric types), or we introduce some abstraction that preserves correctness across backends instead of relying on a loosely defined number.
There was a problem hiding this comment.
Thanks for the feedback @KSDaemon. The frame of reference I'm coming from is that these types need to be precise enough to generate SQL on top of (e.g. a BI tool building on top of the semantic definitions). In my experience, the SQL required for integers vs float/decimal can differ for warehouses that use truncation semantics for integers. I have not come across cases where I've needed to generate different SQL depending on whether the underlying type is a float or decimal.
But it is worth taking a step back to ask what else folks would rely on these logical data types for. Do you have any particular use cases in mind?
There was a problem hiding this comment.
Following up. Are there cases beyond the generation of SQL for semantic queries that you have in mind to support here? I'm happy to add decimal as data type, but I'd like to understand a motivating scenario for how the spec would be used in such a way that number and decimal would be treated differently.
There was a problem hiding this comment.
Using JSON Schema data types (https://json-schema.org/understanding-json-schema/reference/type) as a minimum would be a good idea.
|
Hi @jonmmease ! Thanks for your work on this PR! Some comments on this PR based on comments and discussions we have opened in regards of including spatial semantics in the OSI spec:
On We'd like to make the case for adding Both are first-class native types in the leading cloud data warehouses:
The Leaving these as On The type/role split you've introduced gives spatial data a natural place to - name: geom
datatype: geometry # proposed new enum value
dimension:
spatial: # proposed role marker + metadata
type: polygon
srid: 4326
geographic_level: "census_block_group"
geographic_hierarchy:
parent: "census_tract"
children: []
description: "Block group boundary polygon (WGS84)"
- name: h3
datatype: string # H3 index is stored as a string
dimension:
spatial:
type: spatial_index
index:
system: h3
resolution: 8
rollup_resolutions: [7, 6, 5]
geographic_level: "h3_cell_res8"
description: "H3 cell index at resolution 8"This maps exactly onto the pattern you're establishing: On |
|
Hi @xavipereztr, your descriptions of I'm not opposed to adding a decimal type, but I'd be interested to articulate a case where a consumer of the semantic model would act differently for a float vs decimal column or expression type. You mentioned in the other issue:
If this value is in a column with a decimal type in BigQuery or Snowflake, it's either already truncated or the precision accommodates it. If I'm writing SQL generation logic on top of this dimension, are there cases where this needs to differ when the underlying type is decimal vs a float type? Another factor would be, for translation purposes, whether any mainstream semantic layers already distinguish these numeric types (cube and lookml do not as far as I've seen). If so, that would also be good argument to separate decimal as a dedicated type. Grounding this in a use case would also help me think through whether |
|
Hi @jonmmease - on the On the |
|
I only meant that I think it makes sense to attach the additional |
Add Ontology & Semantic Interoperability as a top-level current effort with links to discussions apache#22, apache#101, apache#108, apache#68 and PRs apache#124, apache#125. Update existing sections with recently opened discussions, PRs, and converters: metric trees (apache#40), primary key semantics (apache#15, apache#119), reusable datasets (apache#103, apache#109), datatype/is_time reframe (PR apache#113), spatial dimension types (apache#114), default_aggregation (apache#115), positive direction (apache#41), physical metadata (apache#110), and new converter PRs for Salesforce (apache#118), dbt (apache#116), and Databricks (apache#120). Also adds CONTRIBUTING.md (apache#122) and working groups page (apache#123) to Developer Experience, and lists merged converters as existing artifacts. Made-with: Cursor
Add Ontology & Semantic Interoperability as a top-level current effort with links to discussions #22, #101, #108, #68 and PRs #124, #125. Update existing sections with recently opened discussions, PRs, and converters: metric trees (#40), primary key semantics (#15, #119), reusable datasets (#103, #109), datatype/is_time reframe (PR #113), spatial dimension types (#114), default_aggregation (#115), positive direction (#41), physical metadata (#110), and new converter PRs for Salesforce (#118), dbt (#116), and Databricks (#120). Also adds CONTRIBUTING.md (#122) and working groups page (#123) to Developer Experience, and lists merged converters as existing artifacts. Made-with: Cursor
| | `boolean` | Logical two-valued truth type. | | ||
| | `date` | Calendar date with no time-of-day component. | | ||
| | `time` | Time-of-day with no date component. | | ||
| | `timestamp` | Instant-in-time without timezone offset (naive / local). | |
There was a problem hiding this comment.
maybe the default timestamp should be with tz (this is the most common use for timestamps to avoid typical errors). then Add a timestamp_ntz for the special case when really no timezone is expected.
There was a problem hiding this comment.
I don't have a strong opinion on it, but I tend to lean towards postgres naming. But also not opposed to omitting timestamp for clarity and only having timestamp_tz and timestamp_ntz
|
Hey all, I'm not up to speed on the decision making process here, or who the decision makers are, but I'd like to do what I can to move this forward. I'm happy to make adjustments to the type system, but not sure we have consensus on this yet. Would it be possible for someone with decision making authority here to summarize the desired changes? |
The Vendors enumeration block was introduced during cherry-pick conflict resolution but was not part of apache/ossie#113. Co-authored-by: Cursor <cursoragent@cursor.com>
|
closing in favor of apache/ossie-temp#12 |
|
Hey @jonmmease Sorry for the confusion here, now the ASF setup is fully done and the original repo is transferred to apache. With that all the open PRs are correct preserved. I am reopening this PR and reviewing now. The PR on the ossie-temp can be closed. |
| # Optional: Logical data type for this metric | ||
| # One of: string, integer, number, boolean, date, time, timestamp, timestamp_tz, other | ||
| # Use "other" + custom_extensions for vendor-specific types | ||
| datatype: string |
There was a problem hiding this comment.
Could you please define the datatype enum in the yaml spec as well ?
I want to make the json spec the source of truth and have the yaml spec be generated via scripts, but until then we will need to preserve the spec in two places unforunately.
khush-bhatia
left a comment
There was a problem hiding this comment.
LGTM, could you please resolve the merge conflicts and one comment on left on keeping the yaml and json version of the spec in sync.
|
The datatypes for geometry/geography can be added in a follow up PR. |
Introduces a top-level `datatype` on Field and Metric with a closed logical enum: string, integer, number, boolean, date, time, timestamp, timestamp_tz, other. Addresses issue apache#84. `datatype` and `dimension.is_time` are independent and orthogonal: - `datatype` declares the field's logical data type (casting/serialization). - `is_time` is a temporal-role marker (time-series analysis, temporal filtering). A field with `is_time: true` may carry any `datatype` (e.g. integer for a year grain, string for a month name, date for a calendar date). When `is_time` is unset, it defaults to `true` for temporal datatypes (`date`, `time`, `timestamp`, `timestamp_tz`) and `false` otherwise. Explicit `is_time` always wins, so authors can set `is_time: false` on an audit `created_at` to keep it off the time axis. Taxonomy and type/role split were chosen after benchmarking 14 peer semantic layers and 5 portable type standards. Notable precedent: Snowflake Semantic Views' YAML authoring form has a `time_dimensions:` collection whose entries can carry any `data_type` (the published example annotates `order_year` with `data_type: NUMBER`); LookML's `dimension_group` accepts `date`, `datetime`, `timestamp`, `epoch`, and `yyyymmdd`. Snowflake converter updated: `_classify_field` honors explicit `is_time` first, then falls back to the temporal-datatype default. 9 new tests cover the datatype paths and the mixed-metadata cases (`d_year` with `datatype: integer` and `is_time: true`, audit timestamp opt-out, etc.). tpcds_semantic_model.yaml demonstrates three coexistence patterns: datatype-only, datatype + is_time, and is_time-only. Also added .gitignore for __pycache__ directories.
60b4abb to
f07e202
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a portable logical datatype attribute for fields and metrics across the core spec, Python models, examples, and multiple converters, while reframing dimension.is_time as an independent temporal role marker (with defaulting behavior when is_time is unset).
Changes:
- Adds
DataType(datatype) to the core schema/spec + Python models and updates examples/docs accordingly. - Updates converters (Snowflake, Salesforce, Polaris, GoodData, dbt/MSI) to preserve/emit
datatypeappropriately while keepingis_timeas a role signal. - Expands/updates test coverage across Python and converter test suites for datatype mapping and effective time-role behavior.
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| python/tests/test_models.py | Adds tests for schema alignment, serialization, and effective time-role defaulting. |
| python/src/ossie/models.py | Introduces OSIDataType, adds datatype to OSIField/OSIMetric, and adds OSIField.is_time_dimension(). |
| python/src/ossie/init.py | Exposes OSIDataType in the public Python API. |
| examples/tpcds_semantic_model.yaml | Annotates example fields/metrics with datatype and demonstrates coexistence patterns. |
| docs/index.md | Updates glossary entry to document Field.datatype and its allowed values. |
| core-spec/spec.yaml | Documents datatype and clarifies dimension.is_time semantics/defaulting. |
| core-spec/spec.md | Adds DataType section, updates Field/Metric docs and examples, and documents type-vs-role guidance. |
| core-spec/osi-schema.json | Adds $defs.DataType and wires datatype into Field/Metric schema definitions. |
| converters/snowflake/tests/test_osi_to_snowflake_yaml_converter.py | Adds datatype mapping tests and classification tests for datatype+role interactions. |
| converters/snowflake/tests/example_converted_tpcds_semantic_model.yaml | Updates expected converted output to include Snowflake data_type emissions for fields. |
| converters/snowflake/src/osi_to_snowflake_yaml_converter.py | Adds datatype mapping to Snowflake data_type and updates _classify_field to respect explicit is_time then datatype defaulting. |
| converters/snowflake/README.md | Documents Ossie datatype → Snowflake data_type mapping and metric datatype omission rationale. |
| converters/salesforce/src/test/resources/examples/osiToSalesforce.yaml | Updates Salesforce example input to include datatype on fields/metrics. |
| converters/salesforce/src/test/java/org/apache/ossie/SalesforceToOsiConverterTest.java | Extends import tests to assert datatype mapping from Salesforce → Ossie. |
| converters/salesforce/src/test/java/org/apache/ossie/OsiToSalesforceConverterTest.java | Extends export tests to validate portable-vs-exact datatype precedence behavior. |
| converters/salesforce/src/test/java/org/apache/ossie/converter/SalesforceDataTypeMapperTest.java | Adds unit tests for Salesforce↔Ossie datatype mapping and compatibility logic. |
| converters/salesforce/src/main/java/org/apache/ossie/converter/SalesforceDataTypeMapper.java | Introduces centralized Salesforce datatype mapping/compatibility utilities. |
| converters/salesforce/src/main/java/org/apache/ossie/converter/MetricMappingHandler.java | Imports Salesforce metric result dataType into Ossie datatype when present. |
| converters/salesforce/src/main/java/org/apache/ossie/converter/FieldMappingHandler.java | Imports field datatype, uses mapper for temporal detection, and applies portable datatype on export with precedence rules. |
| converters/salesforce/src/main/java/org/apache/ossie/converter/ConverterConstants.java | Adds datatype constant and removes hard-coded Salesforce temporal constants. |
| converters/salesforce/README.md | Updates converter docs to describe direction-specific limits and datatype/role behavior. |
| converters/README.md | Documents datatype vs is_time split and adds converter guidance for both fields and metrics. |
| converters/polaris/src/test/java/org/apache/ossie/converter/polaris/OsiPolarisConverterTest.java | Updates Polaris tests for datatype support and exact Iceberg type round-tripping behavior. |
| converters/polaris/src/test/java/org/apache/ossie/converter/polaris/IcebergTypeMapperTest.java | Adds comprehensive tests for Iceberg↔Ossie datatype mapping and nested-ID reassignment. |
| converters/polaris/src/main/java/org/apache/ossie/converter/polaris/PolarisImporter.java | Maps Iceberg physical types to Ossie datatype, preserves exact Iceberg type in custom_extensions, and keeps temporal role classification. |
| converters/polaris/src/main/java/org/apache/ossie/converter/polaris/PolarisExporter.java | Resolves Iceberg types using (1) exact extension, (2) portable datatype, then legacy fallbacks; regenerates nested IDs for exported schemas. |
| converters/polaris/src/main/java/org/apache/ossie/converter/polaris/OsiYamlGenerator.java | Emits datatype and field-level custom_extensions in generated Ossie YAML. |
| converters/polaris/src/main/java/org/apache/ossie/converter/polaris/OsiModelParser.java | Parses datatype and custom_extensions for fields and datasets. |
| converters/polaris/src/main/java/org/apache/ossie/converter/polaris/model/OsiModel.java | Extends Polaris model to include datatype and customExtensions on fields. |
| converters/polaris/src/main/java/org/apache/ossie/converter/polaris/IcebergTypeMapper.java | Introduces shared mapping logic for Iceberg physical types, portable datatypes, exact-type extraction, and nested-ID reassignment. |
| converters/polaris/README.md | Updates mapping reference and documents datatype + exact-type preservation and precedence rules. |
| converters/gooddata/tests/test_roundtrip.py | Adds round-trip coverage for native/unknown/missing GoodData source types via datatype and extensions. |
| converters/gooddata/tests/test_osi_to_gooddata.py | Adds detailed tests for datatype mapping, warnings, and extension precedence on export. |
| converters/gooddata/tests/test_models.py | Adjusts model tests to reflect optional GoodData sourceColumnDataType. |
| converters/gooddata/tests/test_gooddata_to_osi.py | Adds tests for GoodData→Ossie datatype mapping and Opaque preservation behavior. |
| converters/gooddata/src/ossie_gooddata/osi_to_gooddata.py | Uses shared datatype mapping helper; keeps legacy date-dataset heuristic explicitly tied to explicit is_time. |
| converters/gooddata/src/ossie_gooddata/models.py | Makes source_column_data_type optional and updates (de)serialization accordingly. |
| converters/gooddata/src/ossie_gooddata/gooddata_to_osi.py | Adds datatype extraction, sets datatype when present, and preserves unknown types via Opaque + extension. |
| converters/gooddata/src/ossie_gooddata/datatype_mapping.py | Adds centralized GoodData↔Ossie datatype mapping + warning/precedence logic. |
| converters/gooddata/README.md | Documents datatype mapping and limitations (Float/Time/Opaque behaviors). |
| converters/dbt/tests/test_osi_to_msi.py | Adds tests for effective time-role defaulting via datatype + explicit opt-out. |
| converters/dbt/src/ossie_dbt/osi_to_msi.py | Switches time-dimension classification to use field.is_time_dimension(). |
| .gitignore | Adds *.py[cod] ignore pattern. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks @khush-bhatia! I brought the changes I made in the
|
|
@khush-bhatia of course! I will! Thanks! |
AI Disclosure: I developed this PR and its description iteratively with Claude Code and Codex, and have reviewed the resulting changes fully.
Motivation
Closes #84. Related to #17 and follows the earlier discussion in #25.
The issue originally framed this as “use
datatyperather thanis_time.” After reviewing existing semantic-layer implementations, the cleaner model is to represent logical data type and temporal role independently:datatypesays what values a field or metric produces.dimension.is_timesays whether a field plays a temporal-dimension role.For example,
d_year,d_quarter_name, andd_month_namemay have integer data types while all serving temporal roles. Snowflake Semantic Views and LookML similarly separate scalar representation from temporal classification.So, this PR adds
datatypealongsideis_time, then carries that metadata through the Python bindings and existing converter paths that have native type information.What changes
Core specification
Adds an optional top-level
datatypeproperty to bothFieldandMetric, backed by the following logical data types:StringIntegerDecimalFloatBooleanDateTimeDateTimeDateTimeTzOpaqueThe shared names align with the ontology specification’s built-in value types where possible.
Time,DateTimeTz, andOpaqueare additional types. I think these would make sense as ontology value types as well, but this PR doesn't modify the ontology’s layer.The JSON Schema defines the authoritative
DataTypeenum. Until the YAML specification is generated from JSON,spec.yamlmirrors the same enum explicitly.Temporal role semantics
dimension.is_timeremains valid and is documented as independent ofdatatype.For fields with a
dimensionblock:is_timevalue wins.is_timeis unset, it defaults totrueforDate,Time,DateTime, andDateTimeTz, and tofalseotherwise.A field without dimension metadata remains a fact regardless of its datatype. This supports combinations like:
datatype: Integerwithis_time: truefor a year grain.datatype: DateTimeTzwithis_time: falsefor a timestamp that should not be exposed as a time dimension.is_time: truewithoutdatatypefor legacy models that know the role but do not declare a scalar type.Python bindings
OSIDataType.datatypefields toOSIFieldandOSIMetric.OSIField.is_time_dimension()to implement the effective temporal-role rule.Existing converters
I updated the existing converters to propagate data types where it looked unambiguous. Here tagging some folks who have worked on the various converters for visibility.