fix(gooddata): preserve grain for aliased primary-key fields - #347
fix(gooddata): preserve grain for aliased primary-key fields#347mattfaltyn wants to merge 2 commits into
Conversation
Signed-off-by: Matt Faltyn <faltyn.matthew@gmail.com>
| attr = _convert_to_attribute(field_def, ds_name) | ||
| attributes.append(attr) | ||
| if field_name in pk_columns: | ||
| if attr.source_column in pk_columns: |
There was a problem hiding this comment.
This matches PK columns by comparing attr.source_column against pk_columns, which is a string/value match rather than a unique field match. If two fields alias the same physical column (e.g. a natural key customer_id and a display alias customer_key that both resolve to ANSI_SQL: customer_id), both get added to grain_ids, producing a bogus multi-attribute composite grain for what's actually a single-column key.
I suggest keyring grain membership off the field's identity (e.g. matching source columns to specific field defs and taking the first match, or building grain from primary_key entries directly rather than scanning all attributes) instead of plain "is this source_column in pk_columns" check.
There was a problem hiding this comment.
building grain from primary_key entries directly rather than scanning all attributes
Great catch. Grain now follows primary_key entries directly, so each key column resolves to one attribute.
| if not is_date: | ||
| for f in ds.get("fields", []): | ||
| src = _get_source_column(f) | ||
| col_to_attr[src] = f"attr.{ds_name}.{f['name']}" |
There was a problem hiding this comment.
I found it related while reviewing 😄
col_to_attr is a dict keyed by resolved source_column, with no collision check. If two fields resolve to the same physical column, whichever is processed last silently overwrites the earlier entry. Since _convert_relationship looks up target attributes through this map, a relationship meant to reference one attribute can get silently rewired to a different one (no error, just a wrong GdReferenceTarget in the output).
Given _convert_relationship already raises ValueError for an unresolved column, this should probably raise (or at least warn) on a source_column collision here too, rather than overwriting silently.
There was a problem hiding this comment.
this should probably raise on a source_column collision here too
Agreed. The converter now raises ValueError on duplicate resolved source columns, with regression coverage for ambiguous grain and relationship targets.
Signed-off-by: Matt Faltyn <faltyn.matthew@gmail.com>
Summary
primary_keyentries against the converted attribute'ssource_column, which is already derived from the ANSI SQL expression.This keeps the existing behavior for fields whose logical and physical names match while preventing silent loss of key/grain semantics for aliased fields.
Related Issues
Closes #346
Validation
Checklist
Specification
Converters
Documentation
Tests
Compliance