Add intermediary measures for calculated measures - #51
Conversation
This ensures that extend operations (which may reference summarized columns) are executed after the summarize clause in the generated KQL query.
…d hoc measures - Add _find_operator_outside_quotes() helper to find operators not inside quoted strings - Update _escape_and_quote_columns() to recursively escape both sides of operators - Update _is_number_literal() to match integers and decimals with digits on both sides - Add tests for: - Two quoted measures: "Measure 1" + "Measure 2" -> ["Measure 1"] + ["Measure 2"] - Measure with constant: "Measure 1" * 2 -> ["Measure 1"] * 2 - Measure with operator in name: "Measure 1-2" -> ["Measure 1-2"] - _is_number_literal function validation
- Add comprehensive unit tests for new helper functions: - TestIsInsideQuotesOrBrackets (10 tests) - TestFindMatchingParen (8 tests) - TestHasOperatorsOutsideQuotes (8 tests) - TestExtractAndReplaceAggregates (10 tests) - TestContainsAggregateFunction (10 tests) - Add comment explaining existing_aggs registration logic - Apply black formatting
Prevents double-escaping of expressions like (count(a) + count(b)) by detecting matching outer parentheses, processing the inner content recursively, and re-adding the parentheses.
There was a problem hiding this comment.
Pull request overview
Fixes KQL compilation for calculated measures that reference multiple predefined/inline aggregate expressions by introducing intermediary aggregate projections in summarize and rewriting expressions to reference them via extend.
Changes:
- Add aggregate/expression parsing helpers to extract/replace multiple aggregates and detect operators/aggregates in expressions.
- Adjust query assembly to emit
summarizebeforeextend, and preserve select-column ordering in generated KQL. - Expand/update unit tests to cover intermediary aggregate behavior and escaping/operator-detection helpers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
sqlalchemy_kusto/dialect_kql.py |
Implements intermediary aggregate extraction/replacement and reorders emitted KQL parts to prevent mangled/omitted aggregates in calculated measures. |
tests/unit/test_dialect_kql.py |
Updates existing expectations for new clause ordering and adds extensive unit coverage for calculated-measure aggregate extraction and escaping helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not is_alias: | ||
| for operator in ["/", "+", "-", "*"]: | ||
| if operator in name: | ||
| # Split the name at the first operator and wrap the left part | ||
| parts = name.split(operator, 1) | ||
| # Remove quotes if they exist at the edges | ||
| col_part = parts[0].strip() | ||
| if col_part.startswith('"') and col_part.endswith('"'): | ||
| col_part = col_part[1:-1].strip() | ||
| col_part = col_part.replace('"', '\\"') | ||
| return f'["{col_part}"] {operator} {parts[1].strip()}' # Wrap the column part | ||
| # No operators found, just wrap the entire name | ||
| # Find operator that's not inside quotes | ||
| pos = KustoKqlCompiler._find_operator_outside_quotes(name, operator) | ||
| if pos != -1: | ||
| left_part = name[:pos].strip() | ||
| right_part = name[pos + 1 :].strip() | ||
| # Recursively escape both sides | ||
| left_escaped = KustoKqlCompiler._escape_and_quote_columns(left_part) | ||
| right_escaped = KustoKqlCompiler._escape_and_quote_columns( | ||
| right_part | ||
| ) | ||
| return f"{left_escaped} {operator} {right_escaped}" | ||
| # No operators found - strip surrounding quotes if present, then wrap |
There was a problem hiding this comment.
_escape_and_quote_columns recursively splits on - without handling unary minus / negative numeric literals. For example, an expression like "Measure 1" * -1 would split at the leading - in -1 and produce an invalid escaped expression. Suggest treating - as an operator only when it’s binary (e.g., preceded by an operand), and/or extending _is_number_literal to accept an optional leading sign so negative constants aren’t escaped as column names.
There was a problem hiding this comment.
assumption is that negatives don't work
| # group by columns | ||
| by_columns = self._group_by(group_by_cols) | ||
| if has_aggregates or bool( | ||
| by_columns | ||
| ): # Summarize can happen with or without aggregate being created | ||
| summarize_statement = f"| summarize {', '.join(summarize_columns)} " | ||
| if by_columns: | ||
| summarize_statement = ( | ||
| f"{summarize_statement} by {', '.join(by_columns)}" | ||
| ) |
There was a problem hiding this comment.
This code path now aims to preserve column appearance order (lists are used for summarize_columns/extend_columns), but by_columns comes from _group_by() which returns a set. With multiple GROUP BY columns, the summarize ... by ... ordering will be nondeterministic across runs, which can lead to unstable compiled query strings/tests. Consider changing _group_by() (or wrapping its result here) to preserve the original group_by_cols order.
| """Test calculated measure with an ad hoc measure and a constant. | ||
|
|
||
| Measure 1 = count(*), Measure 2 = "Measure 1" * 2 | ||
| Measure 2 should compile to (["Measure 1"]) * 2 (parentheses for arithmetic precedence) |
There was a problem hiding this comment.
This docstring states the compiled output uses parentheses for precedence (e.g., (["Measure 1"]) * 2), but query_expected below does not include parentheses. Please update the docstring to match the current compiler behavior (or adjust the expected output if parentheses are intended).
| Measure 2 should compile to (["Measure 1"]) * 2 (parentheses for arithmetic precedence) | |
| Measure 2 should compile to ["Measure 1"] * 2 |
…ntermediary measures, and parentheses support for calculated measures
Merge After This PR: #50*
Overview
Previously, having predefined measures in a calculated measure would incorrectly generate the query.
Calculated measures using predefined measures without filters would exclude one of the predefined measures in the resulting query. Calculated measures using predefined measures with filters would mangle the query.
The calculated measure with multiple predefined measures would have multiple aggregate expressions; we only matched to one of them during the compilation process, resulting in the other expression getting mangled or omitted.
In this development, we fix this by following this flow:
summarize;extenddoes not support operating on multiple aggregates in one expression.Technical Details
_extract_and_replace_aggregates(): For each aggregate expression within a given expression, add it to the list of intermediary aggregate expressions encountered if it does not already exist in the list. Replace the aggregate expressions in the original expression with their intermediary expressions._get_projection_or_summarize(): Handle 4 difference cases when adding expressions to the query:projectandsummarize. Also add expression to the list of existing aggregates in case it appears in a calculated measure later onextendextend. Add intermediary expressions tosummarizeprojectUI Changes