M kovalsky/dax - #1227
Open
m-kovalsky wants to merge 13 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new sempy_labs.dax subpackage that implements a hand-written DAX tokenizer, parser (producing a small expression AST), pretty-formatter (HTML/notebook display), and a few analysis helpers. It also wires the analysis into TOMWrapper via a new find_non_numeric_aggregations() method that flags measures aggregating non-numeric columns with SUM/AVERAGE/MIN/MAX/PRODUCT (and their *X variants). Two cosmetic reformatting changes to unrelated files are also included.
Changes:
- New
sempy_labs/dax/module:_tokens,_tokenizer,_parser,_expressions,_format,_lineage,_generator,_analysis, plus publicparse_daxandformat_dax. - New
TOMWrapper.find_non_numeric_aggregations()built on the parser. - Tests: one real unit-test file for
_analysis, plus two interactive harnesses (skipped in CI) and a committed generated HTML preview.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sempy_labs/dax/_tokens.py | Enum of DAX token types. |
| src/sempy_labs/dax/_tokenizer.py | Regex-driven tokenizer (param misnamed sql; string-escape regex bug). |
| src/sempy_labs/dax/_parser.py | Pratt-style DAX parser with VAR/RETURN, virtual columns; contains several KEYWORDS typos and a fragile split("["). |
| src/sempy_labs/dax/_expressions.py | AST node classes with walk/find_all/transform/dump. |
| src/sempy_labs/dax/_format.py | HTML color-coded rendering of DAX for notebooks. |
| src/sempy_labs/dax/_lineage.py | Walks AST to extract functions/columns/measures. |
| src/sempy_labs/dax/_generator.py | Minimal AST→DAX printer (partial coverage). |
| src/sempy_labs/dax/_analysis.py | find_numeric_aggregation_columns over the parsed AST. |
| src/sempy_labs/dax/_example.txt | Sample DAX used by interactive test. |
| src/sempy_labs/dax/init.py | Public exports parse_dax, format_dax. |
| src/sempy_labs/tom/_model.py | Adds find_non_numeric_aggregations using the new analysis helper. |
| src/sempy_labs/_generate_semantic_model.py | Cosmetic reformatting of commented-out blocks. |
| src/sempy_labs/semantic_model/_perspective_editor.py | Pure whitespace/line-wrapping changes. |
| tests/test_dax_analysis.py | Unit tests for find_numeric_aggregation_columns. |
| tests/test_parse_dax.py | Interactive (TTY/env-var-gated) harness for parse_dax. |
| tests/test_format_dax.py | Interactive harness; writes an HTML preview into the tests dir. |
| tests/format_dax_preview.html | Generated artifact committed to the repo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+327
to
+349
| for m in self.all_measures(): | ||
|
|
||
| for fn_name, table_name, column_name in find_numeric_aggregation_columns( | ||
| m.Expression | ||
| ): | ||
|
|
||
| data_type = column_data_types.get((table_name, column_name)) | ||
|
|
||
| # Skip unresolved (e.g. virtual columns, typos) and numeric | ||
| # columns - we only want non-numeric model columns. | ||
| if data_type is None or data_type in numeric_types: | ||
| continue | ||
|
|
||
| rows.append( | ||
| { | ||
| "Measure": m.Name, | ||
| "Table": m.Parent.Name, | ||
| "Function": fn_name, | ||
| "Column Table": table_name, | ||
| "Column": column_name, | ||
| "Data Type": str(data_type), | ||
| } | ||
| ) |
Comment on lines
+311
to
+338
| # (table_name, column_name) -> DataType | ||
| column_data_types = { | ||
| (t.Name, c.Name): c.DataType for t in self.model.Tables for c in t.Columns | ||
| } | ||
|
|
||
| columns = { | ||
| "Measure": "string", | ||
| "Table": "string", | ||
| "Function": "string", | ||
| "Column Table": "string", | ||
| "Column": "string", | ||
| "Data Type": "string", | ||
| } | ||
|
|
||
| rows = [] | ||
|
|
||
| for m in self.all_measures(): | ||
|
|
||
| for fn_name, table_name, column_name in find_numeric_aggregation_columns( | ||
| m.Expression | ||
| ): | ||
|
|
||
| data_type = column_data_types.get((table_name, column_name)) | ||
|
|
||
| # Skip unresolved (e.g. virtual columns, typos) and numeric | ||
| # columns - we only want non-numeric model columns. | ||
| if data_type is None or data_type in numeric_types: | ||
| continue |
Comment on lines
+286
to
+301
| """ | ||
| Identifies measures whose DAX expression aggregates a non-numeric | ||
| column via a numeric aggregation function (SUM, SUMX, AVERAGE, | ||
| AVERAGEX, MIN, MINX, MAX, MAXX, PRODUCT, PRODUCTX). | ||
|
|
||
| A column is considered numeric when its TOM DataType is one of | ||
| Int64, Decimal or Double. | ||
|
|
||
| Returns | ||
| ------- | ||
| pandas.DataFrame | ||
| One row per offending column reference with the columns: | ||
| Measure, Table, Function, Column Table, Column, Data Type. | ||
| Returns an empty dataframe (with the same schema) if nothing | ||
| is found. | ||
| """ |
Comment on lines
+142
to
+170
| def format_dax(dax_expression: str, display: bool = True): | ||
| """ | ||
| Color-codes a DAX expression for display in a notebook. | ||
|
|
||
| Uses the DAX parser/tokenizer to classify tokens and renders them with | ||
| a soft, Apple-inspired palette that reads well on both light and dark | ||
| backgrounds: | ||
|
|
||
| * Functions and keywords - soft blue | ||
| * VAR-declared variables - soft teal | ||
| * Numeric literals - soft orange | ||
| * Virtual columns (introduced by ``ADDCOLUMNS`` / ``SELECTCOLUMNS``) - soft pink | ||
|
|
||
| Parameters | ||
| ---------- | ||
| dax_expression : str | ||
| The DAX expression to color-code. | ||
| display : bool, default=True | ||
| If True, displays the formatted DAX in the current notebook and | ||
| returns ``None``. If False, returns an ``IPython.display.HTML`` | ||
| object instead. | ||
|
|
||
| Returns | ||
| ------- | ||
| IPython.display.HTML | None | ||
| The HTML object when ``display=False``; otherwise ``None``. | ||
| """ | ||
|
|
||
| from IPython.display import HTML, display as _display |
Comment on lines
406
to
459
| # Remove mini model annotations from the master model if they exist (cleanup) | ||
| #ann_to_remove = [ | ||
| # ann_to_remove = [ | ||
| # a.Name | ||
| # for a in tom.model.Annotations | ||
| # if a.Name.startswith(icons.prefix_mini) | ||
| #] | ||
| #for ann in ann_to_remove: | ||
| # ] | ||
| # for ann in ann_to_remove: | ||
| # tom.remove_annotation(object=tom.model, name=ann) | ||
|
|
||
| # Set annotations to the master model | ||
| #if filters is not None or perspective is not None: | ||
| # if filters is not None or perspective is not None: | ||
| # with connect_semantic_model( | ||
| # dataset=source_dataset_id, workspace=source_workspace_id, readonly=False | ||
| # ) as tom: | ||
|
|
||
| # ann_name = f"{icons.prefix_mini}_{perspective}" | ||
|
|
||
| # --- Get existing annotation safely --- | ||
| # --- Get existing annotation safely --- | ||
| # try: | ||
| # ann_value = tom.get_annotation_value(object=tom.model, name=ann_name) | ||
| # ann_list = ast.literal_eval(ann_value) if ann_value else [] | ||
| # except Exception: | ||
| # ann_list = [] | ||
|
|
||
| # --- Build lookup (faster than loop) --- | ||
| # --- Build lookup (faster than loop) --- | ||
| # index = {a.get("datasetId"): a for a in ann_list} | ||
|
|
||
| # if target_dataset_id in index: | ||
| # # --- Update existing --- | ||
| # entry = index[target_dataset_id] | ||
| # entry.update( | ||
| # { | ||
| # "datasetName": target_dataset_name, | ||
| # "workspaceId": target_workspace_id, | ||
| # "workspaceName": target_workspace_name, | ||
| # "lastUpdatedDate": now, | ||
| # "filters": filters_value, | ||
| # } | ||
| # ) | ||
| # else: | ||
| # # --- Add new --- | ||
| # ann_list.append( | ||
| # { | ||
| # "datasetId": target_dataset_id, | ||
| # "datasetName": target_dataset_name, | ||
| # "workspaceId": target_workspace_id, | ||
| # "workspaceName": target_workspace_name, | ||
| # "lastUpdatedDate": now, | ||
| # "filters": filters_value, | ||
| # } | ||
| # ) | ||
|
|
||
| # --- Save once --- | ||
| # --- Save once --- | ||
| # tom.set_annotation(object=tom.model, name=ann_name, value=str(ann_list)) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.