Last active
April 12, 2024 19:41
-
-
Save camsaul/23c58d539c274a583580a49a0f6b5846 to your computer and use it in GitHub Desktop.
One namespace with everything related to a clause??
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(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