Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement sum and average aggregation for decimal in SQLite #33721

Merged
merged 5 commits into from
May 29, 2024

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented May 15, 2024

Contributes to #19635

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

@ranma42
Copy link
Contributor Author

ranma42 commented May 15, 2024

Some notes:

  • I ended up duplicating CombineTerms
    private SqlExpression CombineTerms(EnumerableExpression enumerableExpression, SqlExpression sqlExpression)
    {
    if (enumerableExpression.Predicate != null)
    {
    if (sqlExpression is SqlFragmentExpression)
    {
    sqlExpression = _sqlExpressionFactory.Constant(1);
    }
    sqlExpression = _sqlExpressionFactory.Case(
    new List<CaseWhenClause> { new(enumerableExpression.Predicate, sqlExpression) },
    elseResult: null);
    }
    if (enumerableExpression.IsDistinct)
    {
    sqlExpression = new DistinctExpression(sqlExpression);
    }
    return sqlExpression;
    }
    It looks like most IAggregateMethodCallTranslator implementations are re-implementing this same operation, so it might be ok (even intentional to keep them independent), but I was not expecting it at first
  • I am not 100% confident about the handling of nullability:
    • in LINQ .Sum over an empty decimal enumerable returns a decimal 0
    • in LINQ .Sum over an empty decimal? enumerable returns a decimal? null
    • in LINQ .Sum over an empty decimal? enumerable returns a decimal? 0
    • in LINQ .Sum over a decimal? with some null would return null (I believe)
    • in LINQ .Sum over a decimal? with some nulls would ignore all of them
      being unsure, I replicated the handling of SUM
      if (sqlFunctionExpression.IsBuiltIn
      && string.Equals(sqlFunctionExpression.Name, "SUM", StringComparison.OrdinalIgnoreCase))
      {
      nullable = false;
      return _sqlExpressionFactory.Coalesce(
      sqlFunctionExpression.Update(instance, arguments),
      _sqlExpressionFactory.Constant(0, sqlFunctionExpression.TypeMapping),
      sqlFunctionExpression.TypeMapping);
      }
  • adding/removing the CombineTerms step does not affect any of the existing tests; I plan to add at least a test that checks for that, but I might need some guidance in what is the best place to do so (should it be sqlite-specific or generic?)
  • adding/removing the nullability processing changes the SQL output, hence its effect is tested. OTOH, the results of the evaluation seem to pass regardless of that change, which might point out that additional tests are needed for that as well

EDIT: corrected notes on LINQ .Sum() after testing

@ranma42
Copy link
Contributor Author

ranma42 commented May 15, 2024

combined sum+avg as suggested in #33720 (comment)
the max/min aggregation is left for a future PR as it might involve additional design around how to handle other types (ulong, TimeSpan, etc)

@ranma42 ranma42 changed the title Implement sum aggregation for decimal in SQLite Implement sum and average aggregation for decimal in SQLite May 15, 2024
@roji roji requested a review from cincuranet May 15, 2024 15:31
@cincuranet
Copy link
Contributor

@ranma42 Can you please resolve the conflict and then I can merge. Thanks.

@ranma42
Copy link
Contributor Author

ranma42 commented May 28, 2024

@cincuranet rebased 👍

@cincuranet cincuranet merged commit d8fc38e into dotnet:main May 29, 2024
7 checks passed
@cincuranet
Copy link
Contributor

cincuranet commented May 29, 2024

Thanks. Table in #19635 updated.

@ranma42 ranma42 deleted the sqlite-ef_sum branch May 29, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants