Fork me on GitHub
#pathom
<
2020-02-03
>
kszabo01:02:18

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

wilkerlucio15:02:58

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?

kszabo15:02:23

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

wilkerlucio16:02:01

the reason for duplication is only because the code was written in pathom before eql existed

wilkerlucio16:02:39

but cool ideia, did you measure the performance impact with those changes?

kszabo16:02:58

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

kszabo16:02:19

also trying to reduce GC pressure, shenandoah GC helped a bit

kszabo16:02:59

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!

wilkerlucio17:02:21

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 👍

kszabo18:02:34

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.

kszabo18:02:27

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.

👍 4
kszabo01:02:31

or just regular old defn’s

kszabo20:02:07

@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]}]}]

kszabo21:02:51

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?

kszabo21:02:00

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

kszabo21:02:55

the other way would be require clients to always send all inputs, with nil values, but that’s not forward-migratable API design

pithyless21:02:22

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.

pithyless21:02:27

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.

kszabo22:02:13

I don’t see a clean place to put this nilling 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.

kszabo22:02:04

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.

pithyless22:02:37

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)

pithyless22:02:12

But maybe I misunderstood the problem

kszabo22:02:14

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.

kszabo22:02:36

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.

kszabo22:02:12

There is the efficiency argument as well, associng 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.

wilkerlucio23:02:01

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)

❤️ 8
wilkerlucio23:02:55

if anybody is curious, this work is happening here: https://github.com/wilkerlucio/pathom/compare/query-planner

wilkerlucio23:02:04

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)

Krisztian Szabo08:02:20

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.

wilkerlucio10:02:52

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

kszabo10:02:37

Thanks, good 2 know. Maybe with spec2/malli things will change

kszabo10:02:18

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.

wilkerlucio11:02:36

thats a fair point, I think we can instead use a new name, maybe ::input2, but that feels confusing 😛

kszabo11:02:56

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

kszabo11:02:21

I think this is worth considering, I’ll try it out if I get the time

wilkerlucio12:02:26

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

wilkerlucio12:02:53

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)

kszabo12:02:36

I see, fair enough. In my eyes ::input and ::output are very-very close in functionality to fdefs for the resolver fn, just omitting the requirement on the env (which could also be addressed).

wilkerlucio13:02:57

@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?

wilkerlucio13:02:34

or if you are avoiding macros, its all maps in the end 🙂

kszabo13:02:03

Yup, I’m glad it’s built up like it is 🙂 I have envisioned this before with ghostwheel/guardrails, that’s why I’m so attached to the idea.