feat: add MODE aggregate function (#2482) - #2519
Conversation
There was a problem hiding this comment.
Please rename file to test2482.js
| var alasql = require('..'); | ||
| } | ||
|
|
||
| describe('Test MODE aggregate function', function () { |
There was a problem hiding this comment.
Please add test cases that covers
- All NULLs or undefined
- Boolean values
- Strict Equality / Mixed Types: Does MODE treat the number 1 and the string '1' as the same or different?
- Negative Numbers and Floats
|
Thank you for looking into that. As I am not familiar with using MODE for SQL I asked for a bit of input on the PR. Please consider this an inspiration for improvememnts and not everything as a demand. RecommendationImprove the following:
DetailsLifecycle: correct, but make stage 3 explicitThe proposed code implicitly treats “anything other than stage 1 or 2” as finalization: if (!s.length) {
return undefined;
}That works as long as the engine only ever invokes stages 1, 2, and 3, but the conventional AlaSQL implementation should make finalization explicit: if (stage === 3) {
// finalize
}Then either return AlaSQL’s own user-defined aggregate example describes Null behavior: likely correct SQL semanticsThe implementation ignores both if (v !== undefined && v !== null) {
s.push(v);
}That is a sensible choice for a statistical-style The one caveat is AlaSQL’s established quirks and compatibility expectations. The project’s
Main issue: accumulator costThis code stores every non-null input in an array, then creates a s.push(v); // stages 1 and 2
// ...
for (let i = 0; i < s.length; i++) {
// count later
}It is functionally fine, but it needs (O(n)) retained memory per group, which can be expensive for large grouped datasets. That changes the working memory from (O(n)) for all input values to (O(k)), where (k) is the number of distinct non-null values in the group. Tie-breaking needs a deliberate specThe proposal returns the smallest candidate after sorting: candidates.sort((a, b) => {
if (a < b) return -1;
if (a > b) return 1;
return 0;
});
return candidates[0];That is deterministic for ordinary homogeneous numbers or strings, which is good. But it needs a documented policy because “mode” is not inherently single-valued: multiple values can have the same highest frequency. The problem is that JavaScript’s
You should decide one of these API choices:
For a scalar Suggested implementationThis keeps the same user-visible behavior—ignore nullish values, return alasql.aggr.mode = alasql.aggr.MODE = function (v, s, stage) {
if (stage === 1) {
if (v == null) {
return {
counts: new Map(),
maxCount: 0
};
}
return {
counts: new Map([[v, 1]]),
maxCount: 1
};
}
if (stage === 2) {
if (v == null) {
return s;
}
const count = (s.counts.get(v) || 0) + 1;
s.counts.set(v, count);
if (count > s.maxCount) {
s.maxCount = count;
}
return s;
}
if (stage === 3) {
if (!s || s.maxCount === 0) {
return undefined;
}
let result;
let hasResult = false;
for (const [value, count] of s.counts) {
if (count !== s.maxCount) {
continue;
}
if (!hasResult || value < result) {
result = value;
hasResult = true;
}
}
return result;
}
return undefined;
};This avoids creating both a Important edge cases to testAt minimum, I would require tests for: -- One value
SELECT MODE(v) FROM ? -- [{ v: 42 }]
-- 42
-- Typical mode
SELECT MODE(v) FROM ? -- [{ v: 1 }, { v: 2 }, { v: 2 }]
-- 2
-- Tie, assuming lowest-value policy
SELECT MODE(v) FROM ? -- [{ v: 2 }, { v: 1 }, { v: 1 }, { v: 2 }]
-- 1
-- Null values ignored
SELECT MODE(v) FROM ? -- [{ v: null }, { v: 2 }, { v: 2 }, { v: 3 }]
-- 2
-- Entirely nullish input
SELECT MODE(v) FROM ? -- [{ v: null }, { v: undefined }]
-- undefined / SQL NULL
-- Grouped aggregation
SELECT category, MODE(v)
FROM ?
GROUP BY category;
-- Strings
SELECT MODE(v) FROM ? -- ['b', 'a', 'a', 'b']
-- 'a', if lexicographic-lowest tie-breaking is the contractAlso add a test that verifies |
|
Thanks a lot for the detailed feedback @mathiasrw! I'm pretty new to contributing here, so I really appreciate you taking the time to guide me through this and share such helpful suggestions. I've just pushed an update addressing all your points:
Please let me know if anything else needs adjustments! |
Closes #2482
Summary
Adds the
MODE()aggregate function to AlaSQL as requested in #2482.Details
alasql.aggr.MODE(and aliasalasql.aggr.mode) insrc/55functions.js.NULLandundefinedvalues during aggregation.Map.test/test999_mode.jscovering numbers, strings, tie-breaking,GROUP BY, and empty sets.