Metadata schema #23
Metadata schema #23
Conversation
| (dissoc :select) ;; change to select distinct | ||
| (h/select-distinct cols) | ||
| (h/where [:in :target_id target-ids] | ||
| [:in :target_type (map jtypes/as-other target-types)] |
There was a problem hiding this comment.
First common change: jtypes/as-other is how we make things with our enums work properly; we don't need our ->enum-val any more.
| (def avu-columns [:id :attribute :value :unit :target_id :target_type :created_by :modified_by :created_on :modified_on]) | ||
|
|
||
| (defn- avus-base-query | ||
| [cols] | ||
| (-> (apply h/select cols) | ||
| (h/from (t "avus")))) |
There was a problem hiding this comment.
I've been doing a pattern like these two definitions in a bunch of places. Basically, have a plain def with the most common setup of columns, plus a whatever-base-query function that takes a list of columns and selects them from the appropriate table. I'm not sure if it's super useful, but it does cut down a little bit at least. May be worth generalizing, idk
| [:in :target_type (map jtypes/as-other target-types)] | ||
| [:in :attribute attributes] | ||
| [:in :value values]))] | ||
| (plan/select! ds cols (sql/format q)))) |
There was a problem hiding this comment.
This line means:
- turn the query into SQL + parameters
- fetch results from the DB defined by
ds - on each row, run
select-keys cols, and return a collection of the results. Or, if thecolsslot is not a vector of keywords, it will be applied as a function (which also means, passing a single keyword will extract just that column)
| (first (select :avus (where {:id id})))) | ||
| (let [q (-> (avus-base-query avu-columns) | ||
| (h/where [:= :id id]))] | ||
| (plan/select-one! ds avu-columns (sql/format q)))) |
There was a problem hiding this comment.
This is exactly like select! but only does it for the first result row, and doesn't return a collection as a result.
Or basically it's the same as running first on the output of the same call with select!. except probably more efficient I guess
| (jsql/insert-multi! ds | ||
| (t "avus") | ||
| cols | ||
| (map fmt-avu avus)))) |
There was a problem hiding this comment.
The format for these bulk inserts is different than korma. For this, we pass a collection of columns, plus a collection-of-collections, where each entry should be a collection corresponding to the columns listed before.
i.e. (:id :attribute ...) and then [("4609ef48-...." "some-attribute" ...), ...]
| (let [insert-vals (jsql/insert! ds | ||
| (t "comments") | ||
| {:owner_id owner | ||
| :target_id target-id | ||
| :target_type (jtypes/as-other target-type) | ||
| :value comment})] |
There was a problem hiding this comment.
Unlike the multi-insert, the single insert still takes a map of columns to values.
| (jsql/update! ds | ||
| (t "comments") | ||
| {:retracted false | ||
| :retracted_by nil} | ||
| {:id comment-id}) |
There was a problem hiding this comment.
Hopefully fairly self-explanatory how the update works. After DB and table is updates, then conditions.
| (aggregate (count :*) :cnt) | ||
| (where {:target_id target-id :owner_id user})) | ||
| first :cnt pos?)) | ||
| (let [q (-> (h/select [[:> :%count.* 0] :is_favorite]) |
There was a problem hiding this comment.
This is a bit of a change. I made it do SELECT count(*) > 0 AS is_favorite rather than doing the pos? in clojureland.
| (defn search-classes-subselect | ||
| [ontology-version search-term] | ||
| (-> (search-classes-base ontology-version search-term) | ||
| (subselect (fields :iri)))) | ||
| (let [search-term (str "%" (format-query-wildcards search-term) "%")] | ||
| (-> (h/select :iri) | ||
| (h/from (t "ontology_classes")) | ||
| (h/where [:= :ontology_version ontology-version] | ||
| [:ilike :label search-term])))) |
There was a problem hiding this comment.
We weren't using search-classes-base anywhere else, and I think it was an artifact of the select* vs. select vs subselect distinction in korma due to it handling both query generation and execution. With honeysql + next.jdbc they're already separate, so h/select handles all three.
There was a problem hiding this comment.
I also used ilike instead of lowercasing both sides, which is inconsistent with one other spot. I guess we should check if that affects indexing and maybe use the lowercasing if that's better, but dunno
| @@ -198,7 +198,7 @@ | |||
| (defn update-permanent-id-request | |||
| "Records the Permanent ID for a given Request." | |||
| [request-id permanent-id] | |||
| (sql/update :permanent_id_requests | |||
| (ksql/update :permanent_id_requests | |||
There was a problem hiding this comment.
These updates are the bits in progress. I've been changing things over to use the ksql name, since jsql I'm using for next.jdbc. Should go away ultimately anyway.
| :owner_id user}))) | ||
| (-> (h/select :target_id) | ||
| (h/from (t "favorites")) | ||
| (h/where [:in :target_type [:inline target-types]] |
There was a problem hiding this comment.
In the avu namespace [:in :target_type (map jtypes/as-other target-types)] was used.
Is it not needed here, or is this an alternative expression?
There was a problem hiding this comment.
I'm... actually not sure. I did this namespace first, so it could be that I actually have it wrong in the other namespace. I think they might work as alternates though -- jtypes/as-other will mark them correctly to be included as bound parameters, while :inline will inline them in the expression rather than using bind parameters at all. I think that since in raw SQL without bind parameters you can basically treat enums like strings, it might just work out the same.
I think this inline version is a bit cleaner, so when I get a chance to get back to this and test it out, I might just use that everywhere I can.
| (where {:owner_id user | ||
| :target_type [in (map db/->enum-val target-types)]}))) | ||
| (let [q (-> (h/delete-from (t "favorites")) | ||
| (h/where [:in :target_type [:inline target-types]] |
There was a problem hiding this comment.
Same question about [:in :target_type (map jtypes/as-other target-types)] here.
| (let [new-values (mapv #(vector (:ontology_version %) (:iri %) (:label %) (:description %)) class-values)] | ||
| (jsql/insert-multi! ds | ||
| (t "ontology_classes") | ||
| [:ontology_version :iri :label :description] | ||
| new-values))))) |
There was a problem hiding this comment.
This LGTM as-is, but alternatively, if we wanted to define the set of update columns only once:
| (let [new-values (mapv #(vector (:ontology_version %) (:iri %) (:label %) (:description %)) class-values)] | |
| (jsql/insert-multi! ds | |
| (t "ontology_classes") | |
| [:ontology_version :iri :label :description] | |
| new-values))))) | |
| (let [update-cols [:ontology_version :iri :label :description] | |
| new-values (mapv #(mapv (partial get %) update-cols) class-values)] | |
| (jsql/insert-multi! ds | |
| (t "ontology_classes") | |
| update-cols | |
| new-values))))) |
I'm not sure if this way is more confusing, though 🤔
There was a problem hiding this comment.
Hm, I think I like your version a bit more. I was trying to avoid having the same lists multiple times, but I think I was also getting a bit hurried by the time I was doing ontologies and didn't think of doing it that particular way.
This is not complete, but I was instructed to timebox it to the end of today. I think that these changes should all still be acceptable, but without finishing the remaining namespaces we won't be able to use the merged DB. One possible exception: I suspect that if anything I've changed uses transactions from other parts of the code (i.e. outside the persistence namespaces), those transactions will no longer apply. I was planning to do those after finishing the persistence namespace changes.