From d3be4a164bf48a6b620d08fa4f5384d77b31a4da Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Thu, 12 May 2022 12:19:40 -0400 Subject: [PATCH] SearchKit - Pick better default for aggregate functions Improves the metadata for SQL functions as well. --- Civi/Api4/Query/SqlFunctionABS.php | 2 ++ Civi/Api4/Query/SqlFunctionAVG.php | 2 ++ Civi/Api4/Query/SqlFunctionBINARY.php | 2 ++ Civi/Api4/Query/SqlFunctionRAND.php | 2 ++ Civi/Api4/Query/SqlFunctionREPLACE.php | 2 ++ Civi/Api4/Query/SqlFunctionROUND.php | 2 ++ ext/search_kit/ang/crmSearchAdmin.module.js | 29 ++++++++++++++++++- .../crmSearchAdmin.component.js | 4 +-- 8 files changed, 42 insertions(+), 3 deletions(-) diff --git a/Civi/Api4/Query/SqlFunctionABS.php b/Civi/Api4/Query/SqlFunctionABS.php index 7e8b6f1aab..ec543ea4fa 100644 --- a/Civi/Api4/Query/SqlFunctionABS.php +++ b/Civi/Api4/Query/SqlFunctionABS.php @@ -18,6 +18,8 @@ class SqlFunctionABS extends SqlFunction { protected static $category = self::CATEGORY_MATH; + protected static $dataType = 'Integer'; + protected static function params(): array { return [ [ diff --git a/Civi/Api4/Query/SqlFunctionAVG.php b/Civi/Api4/Query/SqlFunctionAVG.php index 44ff3b0720..d385b29eb9 100644 --- a/Civi/Api4/Query/SqlFunctionAVG.php +++ b/Civi/Api4/Query/SqlFunctionAVG.php @@ -18,6 +18,8 @@ class SqlFunctionAVG extends SqlFunction { protected static $category = self::CATEGORY_AGGREGATE; + protected static $dataType = 'Float'; + protected static function params(): array { return [ [ diff --git a/Civi/Api4/Query/SqlFunctionBINARY.php b/Civi/Api4/Query/SqlFunctionBINARY.php index cce4b8ea50..c08fef12f4 100644 --- a/Civi/Api4/Query/SqlFunctionBINARY.php +++ b/Civi/Api4/Query/SqlFunctionBINARY.php @@ -18,6 +18,8 @@ class SqlFunctionBINARY extends SqlFunction { protected static $category = self::CATEGORY_STRING; + protected static $dataType = 'String'; + protected static function params(): array { return [ [ diff --git a/Civi/Api4/Query/SqlFunctionRAND.php b/Civi/Api4/Query/SqlFunctionRAND.php index 8853973eb0..95f44857f2 100644 --- a/Civi/Api4/Query/SqlFunctionRAND.php +++ b/Civi/Api4/Query/SqlFunctionRAND.php @@ -18,6 +18,8 @@ class SqlFunctionRAND extends SqlFunction { protected static $category = self::CATEGORY_MATH; + protected static $dataType = 'Float'; + protected static function params(): array { return [ [ diff --git a/Civi/Api4/Query/SqlFunctionREPLACE.php b/Civi/Api4/Query/SqlFunctionREPLACE.php index 961d151e23..efe1ccabc2 100644 --- a/Civi/Api4/Query/SqlFunctionREPLACE.php +++ b/Civi/Api4/Query/SqlFunctionREPLACE.php @@ -18,6 +18,8 @@ class SqlFunctionREPLACE extends SqlFunction { protected static $category = self::CATEGORY_STRING; + protected static $dataType = 'String'; + protected static function params(): array { return [ [ diff --git a/Civi/Api4/Query/SqlFunctionROUND.php b/Civi/Api4/Query/SqlFunctionROUND.php index 5db43b962c..5bec5c23df 100644 --- a/Civi/Api4/Query/SqlFunctionROUND.php +++ b/Civi/Api4/Query/SqlFunctionROUND.php @@ -18,6 +18,8 @@ class SqlFunctionROUND extends SqlFunction { protected static $category = self::CATEGORY_MATH; + protected static $dataType = 'Float'; + protected static function params(): array { return [ [ diff --git a/ext/search_kit/ang/crmSearchAdmin.module.js b/ext/search_kit/ang/crmSearchAdmin.module.js index a74b1f89be..0364e3e5a6 100644 --- a/ext/search_kit/ang/crmSearchAdmin.module.js +++ b/ext/search_kit/ang/crmSearchAdmin.module.js @@ -180,6 +180,7 @@ var fnName = expr.split('(')[0], argString = expr.substr(fnName.length + 1, expr.length - fnName.length - 2); info.fn = _.find(CRM.crmSearchAdmin.functions, {name: fnName || 'e'}); + info.data_type = (info.fn && info.fn.data_type) || null; function getKeyword(whitelist) { var keyword; @@ -238,6 +239,9 @@ } } }); + if (!info.data_type && info.args.length) { + info.data_type = info.args[0].data_type; + } } // @param {String} arg function parseArg(arg) { @@ -245,11 +249,13 @@ if (arg && !isNaN(arg)) { return { type: 'number', + data_type: Number.isInteger(+arg) ? 'Integer' : 'Float', value: +arg }; } else if (_.includes(['"', "'"], arg.substr(0, 1))) { return { type: 'string', + data_type: 'String', value: arg.substr(1, arg.length - 2) }; } else if (arg) { @@ -262,6 +268,7 @@ value: arg, path: split[0], field: fieldAndJoin.field, + data_type: fieldAndJoin.field.data_type, join: fieldAndJoin.join, prefix: prefixPos > 0 ? split[0].substring(0, prefixPos) : '', suffix: !split[1] ? '' : ':' + split[1] @@ -274,7 +281,7 @@ return; } var splitAs = expr.split(' AS '), - info = {fn: null, args: [], alias: _.last(splitAs)}, + info = {fn: null, args: [], alias: _.last(splitAs), data_type: null}, bracketPos = expr.indexOf('('); if (bracketPos >= 0 && !_.findWhere(CRM.crmSearchAdmin.pseudoFields, {name: expr})) { parseFnArgs(info, splitAs[0]); @@ -282,6 +289,7 @@ var arg = parseArg(splitAs[0]); if (arg) { arg.param = 0; + info.data_type = arg.data_type; info.args.push(arg); } } @@ -330,6 +338,25 @@ parseExpr: parseExpr, getDefaultLabel: getDefaultLabel, fieldToColumn: fieldToColumn, + // Supply default aggregate function appropriate to the data_type + getDefaultAggregateFn: function(info) { + var ret = {flag_before: ''}; + switch (info.data_type) { + case 'Integer': + // For the `id` field, default to COUNT, otherwise SUM + ret.fnName = (info.args[0] && info.args[0].field && info.args[0].field.name === 'id') ? 'COUNT' : 'SUM'; + break; + + case 'Float': + ret.fnName = 'SUM'; + break; + + default: + ret.fnName = 'GROUP_CONCAT'; + ret.flag_before = 'DISTINCT '; + } + return ret; + }, // Find all possible search columns that could serve as contact_id for a smart group getSmartGroupColumns: function(api_entity, api_params) { var joins = _.pluck((api_params.join || []), 0); diff --git a/ext/search_kit/ang/crmSearchAdmin/crmSearchAdmin.component.js b/ext/search_kit/ang/crmSearchAdmin/crmSearchAdmin.component.js index 6b1a1c1a93..3e121a8203 100644 --- a/ext/search_kit/ang/crmSearchAdmin/crmSearchAdmin.component.js +++ b/ext/search_kit/ang/crmSearchAdmin/crmSearchAdmin.component.js @@ -12,7 +12,6 @@ afformLoad, fieldsForJoinGetters = {}; - this.DEFAULT_AGGREGATE_FN = 'GROUP_CONCAT'; this.afformEnabled = 'org.civicrm.afform' in CRM.crmSearchAdmin.modules; this.afformAdminEnabled = 'org.civicrm.afform_admin' in CRM.crmSearchAdmin.modules; this.displayTypes = _.indexBy(CRM.crmSearchAdmin.displayTypes, 'id'); @@ -331,7 +330,8 @@ if (ctrl.canAggregate(col)) { // Ensure all non-grouped columns are aggregated if using GROUP BY if (!info.fn || info.fn.category !== 'aggregate') { - ctrl.savedSearch.api_params.select[pos] = ctrl.DEFAULT_AGGREGATE_FN + '(DISTINCT ' + fieldExpr + ') AS ' + ctrl.DEFAULT_AGGREGATE_FN + '_' + fieldExpr.replace(/[.:]/g, '_'); + var dfl = searchMeta.getDefaultAggregateFn(info); + ctrl.savedSearch.api_params.select[pos] = dfl.fnName + '(' + dfl.flag_before + fieldExpr + ') AS ' + dfl.fnName + '_' + fieldExpr.replace(/[.:]/g, '_'); } } else { // Remove aggregate functions when no grouping -- 2.25.1