Skip to content

Fix #937: count accepts any Measure type and counts Data Points - #957

Merged
albertohernandez1995 merged 5 commits into
1.9.Xfrom
cr-937
Aug 4, 2026
Merged

Fix #937: count accepts any Measure type and counts Data Points#957
albertohernandez1995 merged 5 commits into
1.9.Xfrom
cr-937

Conversation

@albertohernandez1995

Copy link
Copy Markdown
Contributor

Summary

count rejected a Data Set with a Time Measure, raising 1-1-19-12. The guard
against aggregating a TimeInterval Measure ran before the count branch that
discards every Measure and replaces it with int_var, so count was refused a Measure
it never aggregates. The reference manual
types count's operand as a plain dataset, states "Additional Constraints: None", and
counts a String Measure in its own example. count is now exempt from that guard,
alongside the two exemptions the same method already made for it.

Fixing that exposed a second defect in the same operator, agreed as in scope because
the reporter asked for "the number of rows": count skipped any Data Point that had a
null in one of its Measures. Three sites did this — a dropna and a
COUNT(<first Measure>) on the pandas engine, and a COUNT(CASE WHEN ... IS NOT NULL) on DuckDb — so count reported fewer Data Points than the operand held, and
frequently reported null instead of a number. The manual says only that count
"returns the number of the input Data Points".

Several existing test docstrings already recorded this as a defect: "Measure Date with
null, doesn't count the null, we think that should", and "Example that takes the most
left measure".

The two invocation forms have to stay distinct, which is why the manual gives them
separate syntaxes:

form semantics
count ( dataset ) number of Data Points
count ( ) number of Data Points
count ( component ) that Component's non-null values

A component_operand flag threaded from the interpreter keeps the Component form on
its own semantics; without it the Component form would duplicate count().

Checklist

  • Code quality checks pass (ruff format, ruff check, mypy)
  • Tests pass (pytest)
  • Documentation updated (if applicable)

Impact / Risk

  • Behaviour change: count over a Data Set, and count(), now return larger numbers
    wherever the operand holds Data Points with null Measures, and return a number where
    they previously returned null. This is a breaking change for anyone relying on the
    old counts
    , and worth a changelog entry.
  • count no longer raises 1-1-19-12; a Data Set with a Time Measure is accepted.
  • count over a Component is unchanged.
  • 22 reference Data Sets updated across TypeChecking, Aggregate, DWI, UDO, MD_DEMO,
    Bugs and Additional. Each was checked rather than regenerated wholesale: the
    reference-override helper also rewrites unaffected Data Sets, so five files whose only
    change was column order or TimePeriod rendering (2023-01 to 2023-M01) were
    reverted, and two more had their original month formatting restored. Every retained
    change is a count value, spot-checked against the input Data Points.

Notes

tests/Aggregate/GL_466_1 is the clearest illustration: counting per month after
fill_time_series returned null for every month except two, and now returns
12, 28, 31, 30, ... — January 2023 counted from the 20th is 12 days. tests/UDO/GL_442_1
previously had an int_var column that was entirely null.

test_GH_937_1 covers the reported case and fails against the previous code with the
reported 1-1-19-12. Two type-checking tests that asserted that error became result
tests; both already had correct reference Data Sets.

Fixes #937

@albertohernandez1995 albertohernandez1995 self-assigned this Jul 31, 2026
@albertohernandez1995
albertohernandez1995 requested a review from a team July 31, 2026 11:28

@javihern98 javihern98 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to review this one in a call

@javihern98 javihern98 linked an issue Aug 3, 2026 that may be closed by this pull request
2 tasks
count no longer rejecting a Time Measure is the fix #937 asked for. Changing
what count returns for a Data Point with a null Measure is a separate,
breaking change and is tracked on its own; it is reverted here together with
the reference Data Sets it required.

What remains is the guard exemption, its regression test, and the two
type-checking tests that asserted the error and now assert a result against
their original references.
count no longer rejecting a Time Measure is the fix #937 asked for. Changing
what count returns for a Data Point with a null Measure is a separate,
breaking change and is tracked on its own; it is reverted here together with
the reference Data Sets it required.

What remains is the guard exemption, its regression test, and the two
type-checking tests that asserted the error and now assert a result against
their original references.
@albertohernandez1995

Copy link
Copy Markdown
Contributor Author

Need to review this one in a call

Agreed, and I have made that call smaller. This PR bundled two changes of very different
weight, which is what made it hard to review:

  1. count no longer rejecting a Time Measure — the fix Can't use count() with component time #937 asked for, one hunk.
  2. count counting Data Points rather than non-null Measure values — a breaking change to
    returned values, which pulled 22 reference Data Sets with it.

The second is now split out into #NNN and reverted here, together with every reference Data
Set it required. What is left is the guard exemption, test_GH_937_1, and two type-checking
tests that asserted 1-1-19-12 and now assert a result — both against their original,
unmodified references. No count value changes on either engine.

Worth knowing before merge: this fixes the error in #937, but the issue also asks for "the
number of rows", and the number count returns still excludes Data Points with a null Measure.
That is #NNN. I have kept Fixes #937 since the reported error is what the title describes;
happy to switch to Refs #937 and hold the issue open until both land if you prefer.

@javihern98 javihern98 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, thanks!😊

@albertohernandez1995
albertohernandez1995 enabled auto-merge (squash) August 4, 2026 08:38
@albertohernandez1995
albertohernandez1995 merged commit 3d47b2e into 1.9.X Aug 4, 2026
17 checks passed
@albertohernandez1995
albertohernandez1995 deleted the cr-937 branch August 4, 2026 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't use count() with component time

2 participants