This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2020-02-03
Channels
- # aleph (1)
- # announcements (3)
- # aws (36)
- # babashka (35)
- # beginners (25)
- # cider (14)
- # clj-kondo (3)
- # clojure (154)
- # clojure-europe (8)
- # clojure-italy (2)
- # clojure-nl (5)
- # clojure-serbia (1)
- # clojure-uk (133)
- # clojurescript (36)
- # cursive (15)
- # data-science (7)
- # datomic (16)
- # fulcro (34)
- # immutant (9)
- # jackdaw (5)
- # jobs (1)
- # leiningen (39)
- # off-topic (25)
- # pathom (42)
- # planck (13)
- # play-clj (1)
- # re-frame (18)
- # reagent (6)
- # reitit (3)
- # remote-jobs (1)
- # ring-swagger (16)
- # shadow-cljs (67)
- # sql (22)
- # testing (1)
- # uncomplicate (2)
- # vim (21)
- # vscode (6)
is there a reason why https://github.com/edn-query-language/eql is not used for eql parsing inside pathom? potemkin/import-vars
could be used to not break p/ast->query
users for instance
I think right now is a bit mixed, but since they have no difference it should not affect anything, did you find a place where they diverge?
I replaced the predicate based dispatch to protocol-based one in my eql idea pr to improve hot path performance: https://github.com/edn-query-language/eql/pull/9/files#diff-9fe21494d17308f0d9211266c0baa17dR344 I thought eql was the definitive place to do this, I was surprised that pathom had an identical impl. That’s all, nothing major
the reason for duplication is only because the code was written in pathom before eql existed
but cool ideia, did you measure the performance impact with those changes?
nothing too extensive, I’m running integration+load tests currently through the whole parser, trying to get parser overhead down for a 2ms p99 redis resolver so that the whole parser doesn’t take 10ms
haven’t ran criterium benchmarks yet, but this way should be faster. @U055NJ5CC had a good talk about achieving high perfomance for reitit at clojure/north, trying to improve perf. Also I started reviewing your 2.3.0-DEV branch, the new reader looks good!
nice, I'm willing to merge performance improvements back to EQL, we just need to make sure things are really faster and they output the same results 👍
yup, still experimenting with it, for now I’ll keep that PR open until I get to stable state, then I’ll post benchmarks. Feel free to copy stuff over in the meantime if you want. I also dropped dynamic vars as they kill performance and implicit < explicit in my books anyway. I’ll post clj-async-profile graphs as well once I’m done.
I highly recommend you adopt clj-kondo if you haven’t yet, I found many-many lintable small issues in EQL/Pathom, unused aliases and such.
@wilkerlucio looks like I will have to implement optional attributes in resolvers as well 🙂 Just posting here in case you have another idea, with join-context’s I can write this query:
[{#:myservice.input
{:email ""
:shipping-address "foo"
:billing-phone "phone"
:billing-address "addr"
:shipping-phone "shipping"
:ip "127.0.0.1"}
[{:myservice.input-statitistic.groups/all
[:myservice.input-statitistic/id-1
:myservice.input-statitistic/id-2
...
:myservice.input-statitistic/id-177]}]}]
there are more attributes that can be inputs, and there are multiple groupings of ids, each with a separate resolver (of course there are ident based resolvers as well for email->email related statistic ids). The input statistics are not only looked up by themselves, but there are cross checks as well (email + ip) for instance. Therefore it’s nicer for a remote service/Pathom parser to send this initial context. The issue as that the resolver :myservice.input-statitistic.groups/all
currently has to depend on all of the attributes. Do you see of any other way of easing this restriction?
of course I can create a global resolver with no inputs, and fetch the entity for the join context to run what I want, but that’s not really semantic. So optional attribute resolvers seem like the only other way
the other way would be require clients to always send all inputs, with nil values, but that’s not forward-migratable API design
Why not transform the query on the server, before passing it onto the rest of pathom processing? Here, you can merge with default optional nils. Client would never need to know.
Optional attributes in resolvers would break ease-of-understanding when parsing/manipulating the graph (e.g. other tooling, index explorer, etc.), because optional inputs essentially introduce a combinatorial-if and turn your static graph search into a dynamic/conditional one.
I don’t see a clean place to put this nil
ling logic. The groups/all
resolver is no-go, since it won’t be called. I can cook up another attribute like :myservice.default-input/email
that can be provided globally or from :myservice.input/email
via an alias2
. Then resolvers can hard-depend on the default-input
attributes instead.
These are my best ideas, still these introduce attributes in the system that are incidental, that only exist because Pathom has these implementation details/choices/limits.
I was thinking more along the lines of when initializing the pathom parser, add a custom middleware to ::p/plugins
that will modify your query if you detect groups/all
(e.g. via clojure.walk
or specter
)
I think resolvers having an optional component don’t complicate matters too much. There could be ::pc/opt-in
set. Any existing tooling code before updating will see the regular required attributes (if any). The parser just has to be modified to fetch these keys from the entity and merge them together with the required attributes to pass in as params
. The question might come whether to resolve global optional attributes for a resolver that are not in the context if possible, but this can be a parser construction-time/`:pathom/realize-opts #{:opt/foo}` param thing.
I also though of the plugin route, but in my eyes those should be general and not parser specific. Also server-side rewrites of queries is a slippery slope from an understanding standpoint. I would like to keep the solution in the realm of resolvers if possible.
There is the efficiency argument as well, assoc
ing tons of nils into the entity atom map just for the sake of some limitation in the parser seems wasteful. Pathom already generates tons of GC pressure in high throughput scenarios, I would not want to make matters worse.
yeah, I have an idea on my mind on how to do the optionals, they will be part of the new planner thing, but I'm working on more fundamental features on it before (supporting the things that are already supported), the new planner is the thing I described in the Maximal Graph talk, with much better dynamic resolver support, the new planner is way more sophisticated than the current one (the current one can only trace a path for a single attribute, the new all considers all of sibling attributes at once), so the syntax is already defined as well (just don't work) will be something like this: ::pc/input #{:required (p/? :optional)}
, the planner can take this in consideration, as the runner, I think its a not a big problem of explosion because in real systems there isn't usually a lot of options to the same output (at least so far)
if anybody is curious, this work is happening here: https://github.com/wilkerlucio/pathom/compare/query-planner
most of it is working already, and currently I'm working on tools (given the complexity it can happen inside, without tools is quite hard to follow whats going on)
Have you considered using something like spec/keys? The ability to use (or) and (and) inside :req/opt would make it more sophisticated while not increasing the complexity significantly. You can always parse outs input spec objects into indexes the same way to suit the query planner.
in the beginning of pathom they were supported, but the lack of nesting for outputs was a problem, and in the end not having the control didnt worth it, so I prefer to keep a custom definition way, in truth inputs also needs to support nesting eventually
My issue with the (p/? :sys/attr)
optional proposal is that it is a breaking change for libraries/tools which only accounted for a set of keywords in ::pc/input
.
thats a fair point, I think we can instead use a new name, maybe ::input2
, but that feels confusing 😛
well if we figure out a way to use specs/whatever contract system to support nested serializable data descriptions, then we could just use that system’s name like ::spec-input/output
. I would prefer that to reinventing the wheel for a subset of those system’s functionality. Something like data-specs maybe? https://github.com/metosin/spec-tools/blob/master/docs/02_data_specs.md
combined with: https://github.com/metosin/spec-tools/blob/master/docs/03_spec_visitor.md
but honestly Im not excited to get specs integrated on the game again, they work for different concerns I think, specially given that spec dont have a stable api, I prefer to dont complect with it
my motivation for the p/?
is that it works nicely with nested structures too, and its a simple evolution on the current one (a mark for optional, keeping all the same)
I see, fair enough. In my eyes ::input
and ::output
are very-very close in functionality to fdef
s for the resolver fn, just omitting the requirement on the env
(which could also be addressed).
@U08E8UGF7 the design is to make open, if you like some facility like that, I think the way to go is wrap defresolver
with your own macro, in that point you can integrate an external source like spec, makes sense?
or if you are avoiding macros, its all maps in the end 🙂