fix(db): support numeric properties in chart aggregations

[hrsong99]

When creating a Dashboard, there's an issue where aggregating numeric properties (like duration) would crash ClickHouse with Illegal type UInt64 of argument of function notEmpty.

The query builder is applying notEmpty() to all properties to filter out empty strings, but I believe this function throws an error when applied to numeric columns.

Changes:

• Removed the notEmpty() check from the where clause.
• Swapped toFloat64 for toFloat64OrNull.

This should allow numeric columns to pass through correctly while still safely handling empty strings (returning null, which aggregation functions ignore).

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of null values in chart aggregate calculations for sum, average, maximum, and minimum operations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[vercel]

@hrsong99 is attempting to deploy a commit to the Coderax's projects Team on Vercel.

A member of the Team first needs to authorize it.

[CLAassistant]

All committers have signed the CLA.

[coderabbitai]

Walkthrough

Updated aggregation property handling in the getChartSql method of the chart service. Four aggregation branches (sum, average, max, min) now use toFloat64OrNull with simplified null-checking logic instead of toFloat64 paired with additional notEmpty conditions.

Changes

Cohort / File(s)	Summary
Aggregation null-handling refactoring packages/db/src/services/chart.service.ts	Updated four property aggregation branches (property_sum, property_average, property_max, property_min) to use toFloat64OrNull for cleaner null handling; simplified where clause conditions from multi-part checks to single IS NOT NULL requirement

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Changes follow a consistent, repetitive pattern across four similar aggregation branches
• Straightforward null-handling optimization with minimal logic complexity
• Single file with localized scope

Poem

🐰 Four branches grow with cleaner sap,
Where nulls once leaked through logic's gap—
Now OrNull flows, soft and bright,
Aggregations dance in morning light! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(db): support numeric properties in chart aggregations' clearly and specifically describes the main change: enabling numeric properties to work properly in chart aggregation queries by fixing the ClickHouse type error.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

packages/db/src/services/chart.service.ts (1)

176-177: Numeric property aggregations now correctly avoid notEmpty() on numerics

The switch to toFloat64OrNull(${getSelectPropertyKey(event.property)}) in all four aggregation branches plus dropping notEmpty() from the WHERE clause is aligned with ClickHouse patterns and should resolve the Illegal type UInt64 of argument of function notEmpty error while safely handling string/empty values.

One small optional tweak you might consider: instead of (or in addition to) sb.where.property = \${getSelectPropertyKey(event.property)} IS NOT NULL`, predicate on the converted expression, e.g. toFloat64OrNull() IS NOT NULL`, or even omit this extra WHERE since the aggregate already ignores NULLs. That would make it explicit that only successfully castable values are counted, regardless of the underlying column type or map default behavior.

Also applies to: 181-182, 186-187, 191-192

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50ef4c0 and 48c5a72.

📒 Files selected for processing (1)

• packages/db/src/services/chart.service.ts (1 hunks)

[lindesvard]

Thanks! Looks like you have a conflict after we added revenue tracking. Could you resolve it and I'll merge this

[hrsong99]

Done!

[lindesvard]

Okey, so I just tried this, and it still fails.

In this commit 56b01ca I have added isNumericColumn. What we should do is to refactor to use this to do something like this

  if (event.segment === 'property_sum' && event.property) {
    const field = getSelectPropertyKey(event.property);
    if (isNumericColumn(event.property)) {
      sb.select.count = `sum(${field}) as count`;
      sb.where.property = `${field} > 0 AND ${field} IS NOT NULL`;
    } else {
      sb.select.count = `sum(toFloat64OrNull(${field})) as count`;
      sb.where.property = `${field} IS NOT NULL AND notEmpty(${field})`;
    }
  }

[hrsong99]

Sorry about that, I'm currently unable to test the code so it was something I thought might work. I've tried modifying the code to use isNumericColumn, and kept toFloat64OrNull even if column is numeric, since I wasn't sure if the text would always be a float number.

#247