Fork me on GitHub
#clojure-dev
<
2017-06-29
>
seancorfield05:06:08

I'm working on adding reducible queries to clojure.java.jdbc (yay! overdue! JDBC-99 @hiredman @ghadi )... I'd like this to be backward compatible beyond Clojure 1.7. It seems that in 1.7 onward, you can (reify clojure.lang.IReduce ...) and all is well. That doesn't seem to work in 1.5 and 1.6. Am I correct that you need to (reify clojure.core.protocols.CollReduce ...) there? Would that also work in 1.7/1.8/1.9 and if so, are there benefits to using clojure.lang.IReduce instead?

seancorfield05:06:00

(OK, yeah, CollReduce is fine in 1.7... )

Alex Miller (Clojure team)13:06:22

CollReduce is a protocol so you could extend it (not reify it) in Clojure prior to 1.7. That will still work in 1.7+ but certainly it's now preferred to extend IReduce now. In particular reduce and transduce explicitly check for this case as a fast path that bypasses the protocol.

Alex Miller (Clojure team)13:06:20

You could do both as well, although that may add more ambiguity in which path is taken if you end up going through the protocol.

ghadi13:06:00

That's good news to hear @seancorfield . I would advocate avoiding any path which may lead a user to reduce without an initial value. IReduceInit is better in this respect than CollReduce and IReduce

ghadi14:06:04

If that means waiting longer until jdbc has 1.7 as a baseline, I'd wait

ghadi14:06:37

just read the code... tbh I think CollReduce is no bueno anymore. The 2 arity variant (sans init) is underspecified in clojure. Some paths in clojure use (rf) as the init value (c.c.reducers); some use the first item in the realized collection (c.c.protocols/seq-reduce) as the init value

ghadi14:06:25

there was some conference talk where Rich lamented the init-less arity

seancorfield16:06:50

@alexmiller Yeah, I was kind of dinking around in the REPL last night wondering what I could get to work in 1.5.1-1.9.0 and just happened to reify clojure.core.protocols.CollReduce and it worked and the docs for reify say you can reify a protocol so I left it in there…

seancorfield16:06:09

@ghadi Interesting. I initially started down the path of IReduceInit and quickly ran into a case where I wanted to rely on the implicit creation of init from (f). I’ll take another run at it today and see how I feel about it.

seancorfield16:06:02

Ah, clojure.lang.IReduceInit didn’t exist until Clojure 1.7 so it makes it harder to write cross-version library code. Do you have a good argument for why clojure.java.jdbc should not use IReduce @ghadi ? /cc @alexmiller

seancorfield16:06:48

I’m genuinely curious at this point about “why not allow (f) to produce the init if the developer knows what they’re doing?”

Alex Miller (Clojure team)16:06:42

Rich primarily doesn’t like it because it unnecessarily pushes complexity onto f

Alex Miller (Clojure team)16:06:49

@seancorfield fwiw, I think you could just move to Clojure 1.7 as a min version

Alex Miller (Clojure team)16:06:53

as of state of clojure in december, <5% of users were using a version older than Clojure 1.7

seancorfield16:06:38

I may do that before 0.7.0 is released. I’d like to clean up the code in a few places anyway. Up until yesterday, clojure.java.jdbc supported 1.4.0 so I’m not sure I want to drop three versions in one release but I might.

Alex Miller (Clojure team)16:06:05

those users can still use existing versions of java.jdbc, so nothing has been taken away - those people can’t get most of the advantage from reducibles anyways unless they’re on 1.7+

Alex Miller (Clojure team)16:06:53

embrace the future of 2 years ago man

seancorfield16:06:12

True. 😆 I will push out 0.7.0 Beta 1 with the “in-between” code (works on 1.5 onward, uses IReduce on 1.7 onward) and get feedback

seancorfield16:06:15

Thanks for the response via email re clojure.version in pom.xml. Duh! I should’ve known that.

Alex Miller (Clojure team)16:06:34

nah, most people look at that never, easy to not know

ghadi17:06:58

like I said the implicit creation of init from the reducing function is inconsistent at best within clojure

ghadi17:06:28

my general heuristic with reducibles now is to pretend that CollReduce and IReduce don't exist

ghadi17:06:34

only IReduceInit

ghadi17:06:10

IReduce was made a subinterface of IReduceInit for the purpose of backwards compatibility...

seancorfield17:06:11

I pushed out Beta 1. Once it’s on Maven, I’ll announce. And I’ll poll folks about pre-1.7 support.

ghadi18:06:13

historically that's been implemented by using the first item of the collection, not by calling the 0-arity of the rf

ghadi18:06:20

(but again it's been inconsistent and there are counterexamples)

seancorfield18:06:34

Hence the brief survey I’m promoting about java.jdbc: https://www.surveymonkey.com/r/MR2HRFD — if it’s clear that no one wants continued support for pre-1.7 Clojure going forward, I’ll drop all that CollReduce stuff and I’ll also move from IReduce to IReduceInit.

seancorfield18:06:49

I’m only using IReduce in Beta 1 because I didn’t want to deal with compile/load logic to allow code with clojure.lang.IReduceInit to “work” on pre-1.7.

seancorfield18:06:09

(so you already convinced me @ghadi 🙂 )

bronsa21:06:30

@alexmiller thanks for checking on CLJ-1814, I've fixed the regression (I think I introduced it in a refactoring between v1 and v3) and now timings for both found&not-found cases are in line with my original measurments

bronsa21:06:59

re: the IReduce/IReduceInit discussion -- I agree that using (f) to provide init feels weird, as the docstring for reduce reads:

[..]If val is not supplied,  returns the result of applying f to the first 2 items in coll[..]
so the contract of reduce mandates that init will be provided by (f (first coll) (second coll)) ra ther than (f)

bronsa21:06:52

at the same time the opposite is true for transduce, but I see reduce as a guiding principle for IReduce, transduce is just a consumer of reduce & can provide init however it feels like

bronsa21:06:56

I don't 100% agree with @ghadi's position that having just IReduceInit & no IReduce is better an the docstring for reduce never mentions the possibility of failure if init is not provided

bronsa21:06:58

I'd agree with his position if we made a change in reduce's docstring mentioning that reduce can take either coll or reducible & the caveats of reducibles w/o IReduce impls

bronsa21:06:28

building custom reducibles feels to me like a scarcely documented feature of clojure, I'd rather having predictable defaults than hard to understand failures, unless better documentation becomes available. if widely used libraries start exposing reducibles as their api w/o IReduce impls I can see this becoming very confusing for people that don't know about IReduce/IReduceInit

bronsa21:06:49

I only see ambiguity in CollReduce, where the reducers impl use (rf) to provide init, breaking the reduce contract, not with IReduce per se

ghadi21:06:09

there's definitely underspecification and ambiguity which is fine/reality. I'd like to encourage users not to use any init-less reduce and to use the most minimal surface area API (IReduceInit)

bronsa21:06:54

I agree with the sentiment but I don't think we can/should do this unless we mention that reduce w/o init might fail if coll is a custom reducible

ghadi21:06:54

and to avoid having the collection materialize the init value

bronsa21:06:55

ATM not many know about that and there's no official documentation about custom reducibles AFAICT

bronsa21:06:00

with all the discussions around clojure being hostile to newcomers, imagine the experience of one that takes an object documented as being reducible, tries to (reduce f my-thing) expecting that to work as the docstring for reduce promises that it will, and seeing that explode instead

bronsa21:06:25

maybe a warning on the java.jdbc function about it requiring init to reduce would be enough

bronsa21:06:06

but I'd really like to see IReduce/IReduceInit getting some official documentation before contrib libraries start returning just IReduceInit objects

ghadi21:06:47

That makes sense. I'd rather document initless reduce instead of the interfaces tho

bronsa21:06:42

if we want to have libraries start exposing reducibles as APIs we need to document how to make reducibles

bronsa21:06:49

not only how to consume them

ghadi21:06:23

IReduceInit is really clear, and IReduce was a backwards compatibility move.

ghadi21:06:14

New things should only reify IReduceInit

bronsa21:06:38

yeah ok, let's just document IReduceInit then :)

hiredman21:06:29

or not document IReduceInit, and instead add whatever ghadi's latesting reducible producing function is and document that and tell people to use it

ghadi21:06:51

Double heh

ghadi21:06:36

Or write a spec for reduce that complains about calling the wrong arity

hiredman21:06:06

I am not entirely joking, at the repl if you want a set, you can call the 'set' function to create one, you don't reify ISet or something

bronsa21:06:53

yeah I agree that would be a better solution, but looking at the odds of either a documentation change or a new set of functions making it into core, the former has way more chances of happening, historically

bronsa22:06:59

but 100% agreed that an abstraction for creating custom reducibles would be better than documenting interfaces & having people reify them

seancorfield22:06:30

@bronsa’s argument about reduce documenting that without init it uses the first element of the collection and not (f) is pretty compelling… that we are currently in a broken state 🙂

seancorfield22:06:44

In order to implement the no-`init` arity of reduce for the (custom) reducible in java.jdbc, I’d have to duplicate some code and then specifically handle the no-`init` case where the ResultSet ends up being empty (by… throwing an exception perhaps?).

seancorfield22:06:36

Ah, in that case f must accept no arguments and it is called to return the result

seancorfield22:06:44

(dang, that’s a long docstring!)

hiredman22:06:07

that is a thing

seancorfield22:06:55

So, yes, I could certainly Do The Right Thing with implementing IReduce but it would be a pain.

ghadi23:06:30

... and IReduce is not doing the right thing

seancorfield23:06:32

@ghadi In light of reduces docstring specifying exact behavior for both the 2-arity (without init) and the 3-arity (with init), to follow that contract means implementing IReduce, otherwise there’s no 2-arity call and I need to control that in order to correctly implement the contract, no?

seancorfield23:06:39

(reduce f ireduce-init-thing) calls coll-reduce which would then call (.reduce ^clojure.lang.IReduce coll f) which I certainly don’t want: https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/protocols.clj#L88