Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 798eac7b49
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
sidemantic/adapters/holistics.py
Outdated
| aggregation_type = _map_agg_to_aml(metric.agg) | ||
| if metric.sql: | ||
| definition = metric.sql | ||
| elif metric.agg == "count": | ||
| definition = primary_key |
There was a problem hiding this comment.
When exporting a count metric with no sql, the adapter forces definition to the model’s primary_key. In Sidemantic, Metric(agg="count", sql=None) represents COUNT() (see Metric.handle_expr_and_parse_agg), so replacing it with COUNT(primary_key) changes semantics if the PK can be NULL, and it can outright break exports when the model doesn’t actually have the default id column. Consider omitting definition for COUNT() instead of substituting the primary key.
Useful? React with 👍 / 👎.
| if func_name == "rel": | ||
| rel_expr = None | ||
| active = _extract_active_flag(func.args) | ||
| for arg in func.args: | ||
| if isinstance(arg, NamedArg) and arg.name == "rel_expr": | ||
| rel_expr = arg.value | ||
| if active is False: | ||
| return relationships, refs | ||
| rel = _relationship_from_expression(rel_expr, context) |
There was a problem hiding this comment.
The rel() helper only picks up rel_expr from a named argument. If AML uses positional syntax like rel(orders.user_id > users.id, active: true) (a common function-call pattern consistent with relationship()), rel_expr stays None and the relationship is silently dropped. That leads to missing relationships in the parsed graph.
Useful? React with 👍 / 👎.
1 participant
Adds Holistics AML adapter with ANTLR parsing, constants, use, and extend support. Translates core AQL aggregates to SQL for mapped fields. Expands fixtures and comprehensive tests for all mapped model, metric, dimension, and relationship properties. Tests: uv run pytest tests/adapters/holistics.