Type independent gens for TypedPipe/Execution #1918
Conversation
|
This is great! I’ll review in detail this week. Exciting to have this level of test coverage. Cc @non |
There was a problem hiding this comment.
Hey, thanks for tagging me on this.
I had a few questions about the use of type projections in this code, although they don't necessarily indicate problems. Some of the primitive generators seemed a bit weak (no negative values). I'd also consider adding at least one or two case classes which might be generated, just to exercise code paths that aren't dealing with simple types.
The overall approach to type generation looks great!
| @@ -98,9 +100,19 @@ class ExecutionOptimizationRulesTest extends FunSuite with PropertyChecks { | |||
| def write(pipe: Gen[TypedPipe[Int]]): Gen[Execution[TypedPipe[Int]]] = | |||
| pipe.map(_.writeThrough(new MemorySource[Int]())) | |||
|
|
|||
| def write(pipe: Gen[TypedPipe[TypeWith[TypeGen]#Type]], withT: TypeWith[TypeGen]): Gen[Execution[TypedPipe[TypeWith[TypeGen]#Type]]] = { | |||
There was a problem hiding this comment.
I'm somewhat surprised that this type checks.
It seems like the following signature would be just as good or better:
def write(t: TypeWith[TypeGen])(pipe: Gen[TypedPipe[t.Type]]: Gen[Execution[TypedPipe[t.Type]]] = ...There was a problem hiding this comment.
that's will be hard to use with output from anything like Gen[(TypedPipe[_], TypeWith[TypeGen])], but later I would love to see some API on generators to generate a graph with some requirements, like have write at the end.
PS: will remove type projection.
| pipe.hashCode.toLong | ||
| } | ||
|
|
||
| def executionOfStd: Gen[Execution[TypedPipe[TypeWith[TypeGen]#Type]]] = |
There was a problem hiding this comment.
Does the type projection here add anything over Gen[Execution[TypedPipe[_]]]?
I have generally avoided these kinds of projections since I understand Dotty doesn't consider them sound. That said, if this works there may not be a big reason to change it. I was just curious if the type projection added anything?
There was a problem hiding this comment.
not really any reason behind type projection. didn't know about dotty and type projections, thanks.
|
|
||
| object StdGen { | ||
| implicit def num[A](implicit n: Numeric[A], c: Choose[A]): Gen[A] = | ||
| Gen.sized(s => c.choose(n.zero, n.max(n.fromInt(s), n.zero))) |
There was a problem hiding this comment.
Is there a reason to avoid testing negative numbers? It's probably worth having a chance to generate those as well.
There was a problem hiding this comment.
no reason, just wrong approach to have generic gen :) thanks.
| val tupleConverter: TupleConverter[A] = t | ||
| } | ||
|
|
||
| def apply[A, B](a: TypeGen[A], b: TypeGen[B]): TypeGen[(A, B)] = |
| pipeOf(t).map(t -> _) | ||
| } | ||
|
|
||
| def pipeOf(a: TypeWith[TypeGen])(implicit tg: Gen[TypeWith[TypeGen]]): Gen[TypedPipe[a.Type]] = |
There was a problem hiding this comment.
Do you find that this sometimes blows up and generates gigantic graphs?
I often find I need to put a max depth or max size parameter on these kinds of recursive generators. But if this is working for you then it's definitely a lot cleaner to do it this way.
There was a problem hiding this comment.
Yeh, at first run it always blow up with StackOverFlow, with frequency and bigger weight on terminated leaf works now, but we can do it smarter if you like to propose.
|
You may also decide you want a way to make your "key type" generators a bit denser (to ensure you are likely to generate repeated values, allowing your joins to succeed); currently your joins are fairly likely to be empty. I don't think that's needed in this PR but it's just something else that might be useful later on. |
That's would be nice. |
|
Forgot to mention, this PR is more proof of concept to see if we want to invest in this more and migrate all tests on this approach, I'm generally like this but want to see your thoughts. |
|
@dieu Got it, that makes sense. I'll defer to others but (maybe obvious) I'm in favor of moving things to this style. |
|
|
Generators of TypedPipe/Execution inspired by @erik-stripe talk http://plastic-idolatry.com/erik/oslo2019.pdf