You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ossie.apache.org by "iprithv (via GitHub)" <gi...@apache.org> on 2026/10/20 23:21:11 UTC
[PR] fix(sigma): put SumIf/AvgIf/CountDistinctIf field before conditions [ossie]
iprithv opened a new pull request, #621:
URL: https://github.com/apache/ossie/pull/621
## Summary
Sigma documents conditional aggregates with the **field first**:
- `SumIf(field, condition 1, [condition 2], ...)`
- `AvgIf(field, condition)`
- `CountDistinctIf(field, condition 1, [condition 2], ...)`
The Sigma → SQL path treated argument 0 as the condition and argument 1 as the value, so a formula written the documented way produced SQL with the two swapped (wrong numbers, no warning). `AvgIf`, the name Sigma documents, was not recognized at all (`AverageIf` only). Extra conditions were rejected instead of AND-combined.
**Before**
```text
SumIf([Sale], [State] = "TX")
→ SUM(CASE WHEN "Sale" THEN "State" = 'TX' ELSE 0 END)
AvgIf([Sale], [State] = "TX")
→ None
```
**After**
```text
SumIf([Sale], [State] = "TX")
→ SUM(CASE WHEN "State" = 'TX' THEN "Sale" ELSE 0 END)
AvgIf([Sale], [State] = "TX")
→ AVG(CASE WHEN "State" = 'TX' THEN "Sale" END)
CountDistinctIf([Cust], [State] = "TX", [Qty] > 0)
→ COUNT(DISTINCT CASE WHEN "State" = 'TX' AND "Qty" > 0 THEN "Cust" END)
```
## Related Issues
Fixes #606
## Validation
- `uv run pytest` in `converters/sigma`: **108 passed**
- Reproduced the swapped-order failure on unchanged main, then confirmed the documented order, `AvgIf`, and multi-condition forms after the fix
- Fixture `fixtureA_sigma.json` Net Amount formula flipped to the documented `SumIf` order; converter test asserts the ANSI sibling puts `Amount` in the THEN branch
## Checklist
### Specification
- [x] No specification change is required
- [x] No breaking API change beyond correcting previously wrong SQL
### Converters
- [x] Sigma formula translation updated
- [x] Positive and multi-condition regression tests added
- [x] Fixture aligned with Sigma's documented argument order
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sigma): put SumIf/AvgIf/CountDistinctIf field before conditions [ossie]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #621:
URL: https://github.com/apache/ossie/pull/621#issuecomment-6304843302
Thank you for your first contribution to Apache Ossie (incubating)!
A committer will review your PR. In the meantime, please ensure:
- All CI checks pass
- ASF license headers are present on new source files
- Tests cover new functionality
No ICLA is required to contribute. If you are elected as a committer in the future, you will need an [Individual Contributor License Agreement (ICLA)](https://www.apache.org/licenses/contributor-agreements.html) on file with the ASF.
**Questions?** Ask in the PR comments or join us on [Slack](https://join.slack.com/t/apache-ossie/shared_invite/zt-42zw4rflt-Gpve8_NFJq7AsdAQTY~SCg).
We appreciate your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sigma): put SumIf/AvgIf/CountDistinctIf field before conditions [ossie]
Posted by "iprithv (via GitHub)" <gi...@apache.org>.
iprithv commented on PR #621:
URL: https://github.com/apache/ossie/pull/621#issuecomment-6310294581
Filed followup #624 for the SumIf `ELSE 0` → NULL bug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sigma): put SumIf/AvgIf/CountDistinctIf field before conditions [ossie]
Posted by "iprithv (via GitHub)" <gi...@apache.org>.
iprithv commented on code in PR #621:
URL: https://github.com/apache/ossie/pull/621#discussion_r4332514107
##########
converters/sigma/src/ossie_sigma/sigma_formula.py:
##########
@@ -544,14 +551,16 @@ def _build_call(node: FuncCall, dataset_alias: Optional[str]) -> exp.Expression:
if name == "isnotnull" and len(args) == 1:
return exp.Not(this=exp.Is(this=built[0], expression=exp.Null()))
- if name == "sumif" and len(args) == 2:
- return exp.Sum(this=_case(built[0], built[1], exp.Literal.number(0)))
+ if name == "sumif" and len(args) >= 2:
+ return exp.Sum(this=_case(_and_all(built[1:]), built[0], exp.Literal.number(0)))
if name == "countif" and len(args) == 1:
return exp.Count(this=_case(built[0], exp.Literal.number(1), None))
Review Comment:
CountIf isn't field-first (conditions only), so no swap bug.. but it ignored extra conditions. updated it to AND them, matching Sigma's docs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sigma): put SumIf/AvgIf/CountDistinctIf field before conditions [ossie]
Posted by "christianeu-db (via GitHub)" <gi...@apache.org>.
christianeu-db commented on code in PR #621:
URL: https://github.com/apache/ossie/pull/621#discussion_r4332378921
##########
converters/sigma/src/ossie_sigma/sigma_formula.py:
##########
@@ -544,14 +551,16 @@ def _build_call(node: FuncCall, dataset_alias: Optional[str]) -> exp.Expression:
if name == "isnotnull" and len(args) == 1:
return exp.Not(this=exp.Is(this=built[0], expression=exp.Null()))
- if name == "sumif" and len(args) == 2:
- return exp.Sum(this=_case(built[0], built[1], exp.Literal.number(0)))
+ if name == "sumif" and len(args) >= 2:
+ return exp.Sum(this=_case(_and_all(built[1:]), built[0], exp.Literal.number(0)))
if name == "countif" and len(args) == 1:
return exp.Count(this=_case(built[0], exp.Literal.number(1), None))
Review Comment:
should countif also be addressed in this patch?
##########
converters/sigma/tests/test_sigma_formula.py:
##########
@@ -49,7 +49,28 @@
("[Qty] * [Price] + 1", "T", '"Qty" * "Price" + 1'),
("([Qty] + 1) * [Price]", "T", '("Qty" + 1) * "Price"'),
("Median([Amount])", "T", 'PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY "Amount")'),
- ('SumIf([Status] = "won", [Amount])', "T", "SUM(CASE WHEN \"Status\" = 'won' THEN \"Amount\" ELSE 0 END)"),
+ ('SumIf([Amount], [Status] = "won")', "T", "SUM(CASE WHEN \"Status\" = 'won' THEN \"Amount\" ELSE 0 END)"),
Review Comment:
SUM(CASE WHEN <condition> THEN <field> ELSE 0 END) will give the wrong answer when the condition filters everything out. In particular, this form will return 0 instead of NULL ([details](https://stackoverflow.com/questions/12730457/why-does-sum-on-an-empty-recordset-return-null-instead-of-0)).
This could be addressed by instead issuing SUM(CASE WHEN <condition> THEN <field> END) instead (a more explicit but slightly bigger change would be SUM(<field>) FILTER (WHERE <condition>), which was recently added to the portable Ossie expression language.
This issue is pre-existing so could be a followup issue
##########
converters/sigma/src/ossie_sigma/sigma_formula.py:
##########
@@ -429,6 +429,13 @@ def _case(cond: exp.Expression, then: exp.Expression, otherwise: Optional[exp.Ex
return exp.Case(ifs=[exp.If(this=cond, true=then)], default=otherwise)
+def _and_all(parts: list[exp.Expression]) -> exp.Expression:
+ cond = parts[0]
+ for part in parts[1:]:
+ cond = exp.And(this=cond, expression=part)
+ return cond
Review Comment:
nit: could you use the built-in exp.and_ instead of adding this helper? e.g. `exp.and_(*built[1:])` in place of `_and_all(built[1:])`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sigma): put SumIf/AvgIf/CountDistinctIf field before conditions [ossie]
Posted by "christianeu-db (via GitHub)" <gi...@apache.org>.
christianeu-db merged PR #621:
URL: https://github.com/apache/ossie/pull/621
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sigma): put SumIf/AvgIf/CountDistinctIf field before conditions [ossie]
Posted by "christianeu-db (via GitHub)" <gi...@apache.org>.
christianeu-db commented on code in PR #621:
URL: https://github.com/apache/ossie/pull/621#discussion_r4332542908
##########
converters/sigma/tests/test_sigma_formula.py:
##########
@@ -49,7 +49,28 @@
("[Qty] * [Price] + 1", "T", '"Qty" * "Price" + 1'),
("([Qty] + 1) * [Price]", "T", '("Qty" + 1) * "Price"'),
("Median([Amount])", "T", 'PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY "Amount")'),
- ('SumIf([Status] = "won", [Amount])', "T", "SUM(CASE WHEN \"Status\" = 'won' THEN \"Amount\" ELSE 0 END)"),
+ ('SumIf([Amount], [Status] = "won")', "T", "SUM(CASE WHEN \"Status\" = 'won' THEN \"Amount\" ELSE 0 END)"),
Review Comment:
thanks - do you mind filing a followup issue to address the 0 bug?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sigma): put SumIf/AvgIf/CountDistinctIf field before conditions [ossie]
Posted by "iprithv (via GitHub)" <gi...@apache.org>.
iprithv commented on code in PR #621:
URL: https://github.com/apache/ossie/pull/621#discussion_r4334082911
##########
converters/sigma/tests/test_sigma_formula.py:
##########
@@ -49,7 +49,28 @@
("[Qty] * [Price] + 1", "T", '"Qty" * "Price" + 1'),
("([Qty] + 1) * [Price]", "T", '("Qty" + 1) * "Price"'),
("Median([Amount])", "T", 'PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY "Amount")'),
- ('SumIf([Status] = "won", [Amount])', "T", "SUM(CASE WHEN \"Status\" = 'won' THEN \"Amount\" ELSE 0 END)"),
+ ('SumIf([Amount], [Status] = "won")', "T", "SUM(CASE WHEN \"Status\" = 'won' THEN \"Amount\" ELSE 0 END)"),
Review Comment:
sure, done. https://github.com/apache/ossie/issues/624
thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sigma): put SumIf/AvgIf/CountDistinctIf field before conditions [ossie]
Posted by "iprithv (via GitHub)" <gi...@apache.org>.
iprithv commented on code in PR #621:
URL: https://github.com/apache/ossie/pull/621#discussion_r4332517468
##########
converters/sigma/tests/test_sigma_formula.py:
##########
@@ -49,7 +49,28 @@
("[Qty] * [Price] + 1", "T", '"Qty" * "Price" + 1'),
("([Qty] + 1) * [Price]", "T", '("Qty" + 1) * "Price"'),
("Median([Amount])", "T", 'PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY "Amount")'),
- ('SumIf([Status] = "won", [Amount])', "T", "SUM(CASE WHEN \"Status\" = 'won' THEN \"Amount\" ELSE 0 END)"),
+ ('SumIf([Amount], [Status] = "won")', "T", "SUM(CASE WHEN \"Status\" = 'won' THEN \"Amount\" ELSE 0 END)"),
Review Comment:
agreed.. ELSE 0 is pre-existing and can return 0 instead of NULL when nothing matches. happy to follow up with SUM(CASE WHEN ... THEN ... END) (or FILTER (WHERE ...)). leaving it out of this PR so the arg-order fix stays small. thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sigma): put SumIf/AvgIf/CountDistinctIf field before conditions [ossie]
Posted by "iprithv (via GitHub)" <gi...@apache.org>.
iprithv commented on code in PR #621:
URL: https://github.com/apache/ossie/pull/621#discussion_r4332508264
##########
converters/sigma/src/ossie_sigma/sigma_formula.py:
##########
@@ -429,6 +429,13 @@ def _case(cond: exp.Expression, then: exp.Expression, otherwise: Optional[exp.Ex
return exp.Case(ifs=[exp.If(this=cond, true=then)], default=otherwise)
+def _and_all(parts: list[exp.Expression]) -> exp.Expression:
+ cond = parts[0]
+ for part in parts[1:]:
+ cond = exp.And(this=cond, expression=part)
+ return cond
Review Comment:
done, switched to exp.and_ and removed the helper :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org