Skip to content

Instantly share code, notes, and snippets.

@camsaul
Last active April 12, 2024 19:41
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save camsaul/23c58d539c274a583580a49a0f6b5846 to your computer and use it in GitHub Desktop.
Save camsaul/23c58d539c274a583580a49a0f6b5846 to your computer and use it in GitHub Desktop.
One namespace with everything related to a clause??
(ns metabase.lib.clause.avg
(:require
;; I think we should probably actually introduce some sort of interface namespace with all the relevant `defmulti`
;; methods a clause would need to implement so we don't need to `:require` so many different namespaces. Then it's
;; also clear at a glance what sorts of things you can/need to implement for a clause.
[metabase.legacy-mbql.normalize :as mbql.normalize]
[metabase.lib.common :as lib.common]
[metabase.lib.convert :as lib.convert]
[metabase.lib.hierarchy :as lib.hierarchy]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.parser :as lib.parser]
[metabase.lib.schema.aggregation :as lib.schema.aggregation]
[metabase.lib.schema.common :as lib.schema.common]
[metabase.lib.schema.expression :as lib.schema.expression]
[metabase.lib.schema.mbql-clause :as lib.schema.mbql-clause]
[metabase.util.i18n :as i18n]
[metabase.util.malli :as mu]
[metabase.util.malli.registry :as mr]))
;;; I think `define-tuple-mbql-clause` and `define-catn-mbql-clause` are nice, but maybe a little too magical and I'm
;;; not sure they really buy us that much vs. just defining clauses the hard way. The other benefit of this is that it's
;;; super clear what the name of the schema is and where it is defined
(mr/def :mbql.clause/avg
"pMBQL schema for the :avg clause."
[:tuple
#_tag [:= {:decode/normalize lib.schema.common/normalize-keyword} :avg]
#_opts [:ref ::lib.schema.common/options]
#_expr [:ref ::lib.schema.expression/number]])
;;; we could introduce a helper function like this to update the `::lib.schema.mbql-clause/clause` schema when a new
;;; clause is registered.
(lib.schema.mbql-clause/register-clause! :avg :mbql.clause/avg)
(comment
;;; alternatively, maybe we just do something like this for each clause. Then the `::lib.schema.mbql-clause/clause`
;;; can just check and make sure the tag is valid by looking at the hierarchy. If we need to rebuild the `:multi`
;;; schema we can use a watch on the hierarchy to rebuild it whenever the hierarchy changes
(lib.hierarchy/derive :mbql.clause/avg :mbql/clause))
;;; Similarly, while `defop` is nice I think it hides too much stuff and doesn't give you the opportunity to add Malli
;;; schemas for args or add extra info to options like I'm doing below, I think just writing functions out the long way
;;; is ultimately a win.
(mu/defn avg :- :mbql.clause/avg
"Aggregate expression. Return the average of `expr`."
[expr :- ::lib.schema.expression/number]
;; average is always a floating point number... right? avg of two integers 1 and 2 is `1.5`,
[:avg {:lib/uuid (str (random-uuid)), :base-type :type/Float, :effective-type :type/Float}
(lib.common/->op-arg expr)])
;;; average always returns a Float!!!!
(defmethod lib.schema.expression/type-of-method :avg
[_tag _opts _expr]
:type/Float)
(defmethod lib.metadata.calculation/column-name-method :avg
[_query _stage-number _clause]
"avg")
(defmethod lib.metadata.calculation/column-name-method :avg
[query stage-number [_tag _opts arg] style]
(let [arg-display-name (lib.metadata.calculation/display-name query stage-number arg style)]
(i18n/tru "Average of {0}" arg-display-name)))
;;; this doesn't exist yet, but we could add a method to define these.
(defmethod lib.schema.aggregation/operator-info :avg
[_tag]
{:short :avg
:supported-field :metabase.lib.types.constants/summable
:requires-column? true
:driver-feature :basic-aggregations
:display-info (fn []
{:display-name (i18n/tru "Average of ...")
:column-name (i18n/tru "Average")
:description (i18n/tru "Average of all the values of a column")})})
;;; now that we have pMBQL normalization done with Malli decoders I think we should consider whether we want to move
;;; `->legacy-MBQL` into the schema as well, using Malli encoders. If we're using `:multi` schemas then I think we could
;;; have it be reasonably performant, and the benefit is shared schemas like `::lib.schema.common/options` can define
;;; their encoding logic once and every clause that has an options map gets that logic for free.
;;;
;;; `->pMBQL` could probably also be done with a Malli decoder without too much drama if we wanted.
(defmethod lib.convert/->legacy-MBQL :avg
[_tag _opts expr]
[:avg (lib.convert/->legacy-MBQL expr)])
(defmethod lib.convert/->pMBQL :avg
[[_tag expr]]
(avg (lib.convert/->pMBQL expr)))
;;; We could add a method for this too and port the FE `helper-text-strings.ts` to MLv2
(defmethod lib.parser/helper-text :avg
[_tag]
{:name "avg"
:description (i18n/tru "Returns the average of the values in the column.")
:args [{:name (i18n/tru "column")
:description (i18n/tru "The numeric column whose values to average.")
:example (i18n/tru "Quantity")}]})
;;; Legacy stuff, I think this should live here too so everything really is in one place. Not sure tho.
(mr/def :metabase.legacy-mbql.schema/avg
"Legacy MBQL schema for the :avg clause."
#_tag [:= :avg]
#_expr [:ref :metabase.legacy-mbql.schema/FieldOrExpressionDef])
(defmethod mbql.normalize/normalize :avg
[_tag expr]
[:avg (mbql.normalize/normalize expr)])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment