search

tomjaguarpaw / haskell-opaleye

Other
599 stars 115 forks source link

Add optionalInternal #514

Closed tomjaguarpaw closed 3 years ago

tomjaguarpaw commented 3 years ago

In the hope of avoiding drift between Opaleye and Rel8 I'd like to offer a generic version of optional called optionalInternal which should be sufficient to replace your optional with. I suspect it would be

optional = optionalInternal (MaybeTable . fromExpr)

[EDIT: Oh, it's not quite that because you'd have to hit it with Column a -> Expr b.]

Is that something you would take advantage of? If so then I will merge this and put it in the same release as the LATERAL stuff.

cc @duairc

duairc commented 3 years ago

I don't think that would be the same. We don't run the tag through isNotNull. Is your concern here that we're using too much of Opaleye's internals to implement optional and you would hesitate to make changes for fear of breaking us?

If that's the case, then maybe alignBy might be a concern too, which uses a similar trick for full outer joins.

tomjaguarpaw commented 3 years ago

I don't think that would be the same. We don't run the tag through isNotNull

Ah yes, OK. Fixed that.

Is your concern here that we're using too much of Opaleye's internals

No, I'm not worried about breaking internal modules. I'm sure you can deal with that (as you did with the internal changes for lateral, for example, although if you were already depending on optionalInternal that change would have been smaller).

I just think that if we're going to have equivalent functionality then it seems like bad practice to allow the implementations to be able to drift. For example

  1. We're going to run into situations where users say "I ran this in Opaleye and it worked fine but in Rel8 it's broken" (or vice versa), and
  2. One version will silently become worse (because it's less used) than the other one.

If we could do it the other way round and I could depend on Rel8's implementation then I would! But give that that is impossible it seems like the best thing to do is for them both to depend on a common implementation that must be in Opaleye.

(alignBy is very cool by the way. Do you actually have a practical use case? I've never seen a real use case for FULL OUTER JOINS.)

tomjaguarpaw commented 3 years ago

A case in point: I'd like to be able to do simplifications like 8c2afc92 (which would also be useful for alignBy) without worrying that you are making independent improvements to Rel8's optional, thus diverging the two implementations.

I don't think there's anything Opaleye or Rel8 specific about things like optional. It would be good to share the implementation.