Skip to content

feat(expressions): support count(mode='all') without expr#6358

Open
Abyss-lord wants to merge 1 commit intoEventual-Inc:mainfrom
Abyss-lord:5526-count-mode-all
Open

feat(expressions): support count(mode='all') without expr#6358
Abyss-lord wants to merge 1 commit intoEventual-Inc:mainfrom
Abyss-lord:5526-count-mode-all

Conversation

@Abyss-lord
Copy link
Contributor

Changes Made

This PR adds support for calling daft.functions.count(mode="all") without passing an expression, enabling free-standing row-count aggregations similar to SQL COUNT(*).

What changed

  • Updated daft.functions.count in daft/functions/agg.py to accept an optional expr argument.
  • Added explicit behavior for expr=None:
    • mode="all" is allowed and maps to count(*) semantics via col("*").
    • mode="valid" or mode="null" now raises a clear ValueError when no expression is provided.
  • Preserved backward compatibility for all existing count(expr, mode=...) and count(lit(...)) usages.

Tests added

In tests/dataframe/test_aggregations.py:

  • Global aggregation with count(mode="all") and no expression.
  • Grouped aggregation with count(mode="all") and no expression.
  • Empty DataFrame behavior (0 result).
  • Error path validation for no-expression calls with non-all modes.

Validation run

  • DAFT_RUNNER=native make test EXTRA_ARGS="-v tests/dataframe/test_aggregations.py"
  • DAFT_RUNNER=native make test EXTRA_ARGS="-v tests/sql/test_sql.py tests/sql/test_exprs.py"

Related Issues

Closes #5526

@github-actions github-actions bot added the feat label Mar 6, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 6, 2026

Greptile Summary

This PR extends daft.functions.count to accept an optional expr argument, enabling count(mode="all") as a free-standing COUNT(*) equivalent — closing issue #5526.

Key changes:

  • expr in daft/functions/agg.py is made optional (defaults to None); when None and mode="all", the implementation substitutes col("*") and delegates to the existing Expression._expr.count(mode) path.
  • A ValueError is raised for expr=None combined with mode="valid" or mode="null", since those semantics require a concrete column.
  • The col symbol is added to the module-level import in agg.py.
  • Four new tests cover global aggregation, grouped aggregation, empty DataFrames, and the error path.

The implementation is straightforward, with tests demonstrating correct behavior across various partition configurations and edge cases including empty DataFrames.

Confidence Score: 4/5

  • The PR is safe to merge. Changes are minimal and focused, with comprehensive test coverage including edge cases.
  • The feature is small, well-scoped, and thoroughly tested. The implementation correctly adds optional expr support to count() with proper backward compatibility. Tests verify correct behavior across single and multi-partition scenarios, including empty DataFrames and error cases. No breaking changes are introduced.
  • No files require special attention

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["count(expr=None, mode=CountMode.Valid)"] --> B{is mode a string?}
    B -- yes --> C["mode = CountMode.from_count_mode_str(mode)"]
    B -- no --> D{expr is None?}
    C --> D
    D -- no --> G["Expression._from_pyexpr(expr._expr.count(mode))"]
    D -- yes --> E{mode == CountMode.All?}
    E -- no --> F["raise ValueError('count() without an expression\nonly supports mode=all')"]
    E -- yes --> H["expr = col('*')"]
    H --> G
    G --> I["return Expression"]
Loading

Last reviewed commit: 5a4da66

@Abyss-lord
Copy link
Contributor Author

Abyss-lord commented Mar 8, 2026

@rchowell @universalmind303 could you please review this PR when you have time? I’d really appreciate your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support daft.functions.count(mode='all')

1 participant