Repository navigation
fix(gooddata): preserve grain for aliased primary-key fields #347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4be8045
55d0c0b
55e152a
a685310
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -330,6 +330,132 @@ def test_grain_from_primary_key(ossie_tpcds_dict: dict): | |||||||||||||||||||||||||||||||||||||||
| assert len(grain_ids) == 2 | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| @pytest.mark.parametrize( | ||||||||||||||||||||||||||||||||||||||||
| "primary_key", | ||||||||||||||||||||||||||||||||||||||||
| [["customer_id"], ["customer_id", "customer_id"]], | ||||||||||||||||||||||||||||||||||||||||
| ids=["single-key", "repeated-key"], | ||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||
| @pytest.mark.parametrize( | ||||||||||||||||||||||||||||||||||||||||
| "field", | ||||||||||||||||||||||||||||||||||||||||
| [ | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| "name": "customer_key", | ||||||||||||||||||||||||||||||||||||||||
| "expression": {"dialects": [{"dialect": "ANSI_SQL", "expression": "customer_id"}]}, | ||||||||||||||||||||||||||||||||||||||||
| "dimension": {}, | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| "name": "customer_key", | ||||||||||||||||||||||||||||||||||||||||
| "expression": { | ||||||||||||||||||||||||||||||||||||||||
| "dialects": [ | ||||||||||||||||||||||||||||||||||||||||
| {"dialect": "ANSI_SQL", "expression": "customer_id"}, | ||||||||||||||||||||||||||||||||||||||||
| {"dialect": "MAQL", "expression": "{label/customers.customer_key}"}, | ||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||
| ids=["dimension", "maql-attribute"], | ||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||
| def test_grain_uses_source_column_for_aliased_attribute(field: dict, primary_key: list[str]): | ||||||||||||||||||||||||||||||||||||||||
| """Verify physical primary keys select aliased GoodData grain attributes.""" | ||||||||||||||||||||||||||||||||||||||||
| model = { | ||||||||||||||||||||||||||||||||||||||||
| "name": "m", | ||||||||||||||||||||||||||||||||||||||||
| "datasets": [ | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| "name": "customers", | ||||||||||||||||||||||||||||||||||||||||
| "source": "db.s.customers", | ||||||||||||||||||||||||||||||||||||||||
| "primary_key": primary_key, | ||||||||||||||||||||||||||||||||||||||||
| "fields": [field], | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| customer = ossie_to_gooddata(model).ldm.datasets[0] | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| assert customer.attributes[0].source_column == "customer_id" | ||||||||||||||||||||||||||||||||||||||||
| assert [grain.id for grain in customer.grain] == ["attr.customers.customer_key"] | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def test_duplicate_source_columns_are_rejected(): | ||||||||||||||||||||||||||||||||||||||||
| """Verify ambiguous grain and relationship targets fail instead of being misassigned.""" | ||||||||||||||||||||||||||||||||||||||||
| model = { | ||||||||||||||||||||||||||||||||||||||||
| "name": "m", | ||||||||||||||||||||||||||||||||||||||||
| "datasets": [ | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| "name": "customers", | ||||||||||||||||||||||||||||||||||||||||
| "primary_key": ["customer_id"], | ||||||||||||||||||||||||||||||||||||||||
| "fields": [ | ||||||||||||||||||||||||||||||||||||||||
| _direct_field("customer_id", dimension={}), | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| "name": "customer_key", | ||||||||||||||||||||||||||||||||||||||||
| "expression": { | ||||||||||||||||||||||||||||||||||||||||
| "dialects": [{"dialect": "ANSI_SQL", "expression": "customer_id"}] | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| "dimension": {}, | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| "name": "orders", | ||||||||||||||||||||||||||||||||||||||||
| "fields": [_direct_field("customer_id", dimension={})], | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||
| "relationships": [ | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| "name": "orders_customer", | ||||||||||||||||||||||||||||||||||||||||
| "from": "orders", | ||||||||||||||||||||||||||||||||||||||||
| "to": "customers", | ||||||||||||||||||||||||||||||||||||||||
| "from_columns": ["customer_id"], | ||||||||||||||||||||||||||||||||||||||||
| "to_columns": ["customer_id"], | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe worth to add a test case verifying that a dataset with an attribute and a fact referencing the same physical column (or two facts on the same column) converts without raising an error. Something like:
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion. Added a regression with one attribute and two facts sharing a source column, including grain and relationship target checks. |
||||||||||||||||||||||||||||||||||||||||
| with pytest.raises( | ||||||||||||||||||||||||||||||||||||||||
| ValueError, | ||||||||||||||||||||||||||||||||||||||||
| match="Dataset 'customers': source column 'customer_id' maps to multiple attributes", | ||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||
| ossie_to_gooddata(model) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def test_attribute_and_facts_sharing_source_column_are_permitted(): | ||||||||||||||||||||||||||||||||||||||||
| """Facts sharing a source column do not make grain or reference targets ambiguous.""" | ||||||||||||||||||||||||||||||||||||||||
| fact = _direct_field("customer_id") | ||||||||||||||||||||||||||||||||||||||||
| model = { | ||||||||||||||||||||||||||||||||||||||||
| "name": "m", | ||||||||||||||||||||||||||||||||||||||||
| "datasets": [ | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| "name": "customers", | ||||||||||||||||||||||||||||||||||||||||
| "primary_key": ["customer_id"], | ||||||||||||||||||||||||||||||||||||||||
| "fields": [ | ||||||||||||||||||||||||||||||||||||||||
| _direct_field("customer_id", dimension={}), | ||||||||||||||||||||||||||||||||||||||||
| {**fact, "name": "customer_count"}, | ||||||||||||||||||||||||||||||||||||||||
| {**fact, "name": "customer_total"}, | ||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| "name": "orders", | ||||||||||||||||||||||||||||||||||||||||
| "fields": [_direct_field("customer_id", dimension={})], | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||
| "relationships": [ | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| "name": "orders_customer", | ||||||||||||||||||||||||||||||||||||||||
| "from": "orders", | ||||||||||||||||||||||||||||||||||||||||
| "to": "customers", | ||||||||||||||||||||||||||||||||||||||||
| "from_columns": ["customer_id"], | ||||||||||||||||||||||||||||||||||||||||
| "to_columns": ["customer_id"], | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| customers, orders = ossie_to_gooddata(model).ldm.datasets | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| assert len(customers.attributes) == 1 | ||||||||||||||||||||||||||||||||||||||||
| assert len(customers.facts) == 2 | ||||||||||||||||||||||||||||||||||||||||
| assert [grain.id for grain in customers.grain] == ["attr.customers.customer_id"] | ||||||||||||||||||||||||||||||||||||||||
| assert orders.references[0].sources[0].target.id == "attr.customers.customer_id" | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def test_relationships_become_references(ossie_tpcds_dict: dict): | ||||||||||||||||||||||||||||||||||||||||
| """Verify Ossie relationships become GoodData references.""" | ||||||||||||||||||||||||||||||||||||||||
| result = ossie_to_gooddata(ossie_tpcds_dict) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mainhas been updated removing the rootsemantic_model: [...]wrapper list.Once rebased onto
main, these tests fail with:Please rebase on
mainand flatten the test fixtures to putname,datasets, andrelationshipsat the root:(same update needed in
test_duplicate_source_columns_are_rejectedbelow)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Rebased onto current main and flattened both test fixtures. The full GoodData suite passes on Python 3.12, 3.13, and 3.14.