Produce even better DOT graphs #3

benesch · 2016-11-10T22:53:53Z

Output DOT graphs without using petgraph's stock formatter so that we
can output each node's configuration using a dense, human-readable,
relational-algebra-like format.

Here's what the main/web graph looks like in this new world:

ms705

This looks good to me in principle; I left some minor comments, but I think we should merge this quickly as it will be useful to everyone.

ms705 · 2016-11-11T03:43:24Z

src/flow/mod.rs

@@ -326,6 +327,35 @@ impl<Q, U, D> FlowGraph<Q, U, D>
        (&self.graph, self.source)
    }

+    /// Write a DOT representation of the graph to `f`.
+    pub fn write_dot<W: io::Write>(&self, f: &mut W) -> io::Result<()> {


This is a minor comment, but would it make sense to either

put this implementation into impl Display for FlowGraph<...> (or Debug rather than Display), such that one can simply print a flow graph to get the dot graph? (AIUI, your PR leaves the old behavior of printing a simplistic graph in place?); and/or

change the second argument here to be fmt::Formatter, rather than a generic type that implements io::Write?

I think (1) we should do for sure, but (2) is a minor stylistic point and your code may even be preferable as it is more general and e.g., supports direct writing to a file.

Happy to do what you think is best! I chose this method because I figured—at least for Debug—a dot graph might not be the most useful output format for immediate debug insight.

Regarding (2), how does one create an instance of fmt::Formatter without calling format!? It seems like you can only get one of those by implementing Display or Debug.

ms705 · 2016-11-11T03:44:48Z

src/ops/grouped/aggregate.rs

@@ -145,6 +155,12 @@ mod tests {
    }

    #[test]
+    fn it_describes() {
+        let c = setup(true, true);
+        assert_eq!(c.inner.description(), "Count γ[0, 2]");


Nit: might be good to test the SUM case too here, since that produces a somewhat different output.

ms705 · 2016-11-11T03:46:07Z

src/ops/join.rs

+            .flat_map(|(left, rs)| {
+                rs.against
+                    .iter()
+                    .filter(move |&(right, _)| left < right)


Why is this move?

The borrow checker complains otherwise

error: `left` does not live long enough --> src/ops/join.rs:449:43 | 449 | .filter(|&(right, _)| left < right) | ------------- ^^^^ does not live long enough | | | capture occurs here ... 457 | }) | - borrowed value only lives until here 458 | .collect::<Vec<_>>() | - borrowed value needs to live until here

and I wasn't sure how else to fix it. Thoughts?

Ah, I see -- yes, that seems fine. (I guess you could technically borrow left in the inside closure, i.e., use &left, but that wouldn't be much prettier.)

Yeah, this is a somewhat annoying side-effect of using flat_map. move is perfectly acceptable to use here.

benesch · 2016-11-11T06:01:32Z

Okay, I made this Display and restored the original implementation of Debug. This has the nice side effect of making debug printouts of Node objects more what you'd expect: a near-complete listing of the struct's fields. But let me know if you'd rather I nuke the old Debug completely.

jonhoo · 2016-11-11T16:10:30Z

I don't think I currently have a strong preference for what Debug looks like, as long as it's somewhat readable and has most of the interesting information. Arguably this should not use Display, but rather some new trait like Draw, but since we're not using Display for anything, it's probably fine.

As for the visualization, I'm not entirely sure I understand the top-right notation in each box, but that's probably just due to my own ignorance. Including a legend somewhere would be useful though.

jonhoo · 2016-11-11T16:28:35Z

src/flow/mod.rs

+        use ops::NodeOp;
+        let indent = "    ";
+        try!(writeln!(f, "digraph {{"));
+        try!(writeln!(f, "{}node [shape=record, fontsize=10]", indent));


Since Rust 1.13, you can now use writeln!(..)? instead if you think that reads better.

indent seems unnecessary here, no?

indent helps produce more human-readable dot graphs, which is useful for debugging them ;-)

maybe, though the indent seems pretty basic (you could get the same output in vim by typing gg=G), and it makes the code harder to read.

I reformatted the function to use an indent lambda that should make the code easier to read?

jonhoo · 2016-11-11T16:29:48Z

src/ops/base.rs

@@ -65,4 +65,8 @@ impl NodeOp for Base {
    fn is_base(&self) -> bool {
        true
    }
+
+    fn description(&self) -> String {
+        "𝓕".into()


Any particular reason for the choice of 𝓕 to signify base nodes?

And if so, maybe include a comment somewhere pointing to a resource laying out the "known" operators

F for fact. We could also do 𝓑 for base, or "Base"?

I prefer "B" for base. I don't think it should be stylized, because that makes me think that is some "well known" operator from relational algebra or something.

jonhoo · 2016-11-11T16:31:32Z

src/ops/grouped/aggregate.rs

+    fn description(&self) -> String {
+        let op_string = match self.op {
+            Aggregation::COUNT => "Count".into(),
+            Aggregation::SUM => format!("Sum({})", self.over),


Should "Sum" not be 𝛴 ? I don't know if there's an equivalent symbol for "Count"

Right, and I figured min and max are usually spelled out too, so I left it as "Sum" for consistency.

But I suppose we could use ∧ for min and ∨ for max, and |*| for count (magnitude bars). I don't have strong feelings.

I like |*| for count. For min/max, I feel like T (top) and ⊥ (bottom) could work. We could also use ⌈x⌉ (ceiling) and ⌊x⌋ (floor) where x is the column being computed over.

Sounds good. Done!

jonhoo · 2016-11-11T16:31:58Z

src/ops/grouped/aggregate.rs

+        };
+        let group_cols = self.group.iter().map(|g| g.to_string())
+            .collect::<Vec<_>>().join(", ");
+        format!("{} γ[{}]", op_string, group_cols)


Why the γ ?

Relational algebra for "group by", so that's how it reads in my head.

Ah, I see, so the γ here actually does stem from some definition. A reference would be useful. It doesn't need to be in the diagram, but at least somewhere in the code. Maybe in the docstring for the description method in the trait definition.

jonhoo · 2016-11-11T16:32:56Z

src/ops/grouped/concat.rs

+        let group_cols = group_cols.iter().map(|g| g.to_string())
+            .collect::<Vec<_>>().join(", ");
+
+        format!("Concat({} / \"{}\") γ[{}]", fields, self.separator, group_cols)


I think || is often used to signify concatenation

jonhoo · 2016-11-11T16:35:07Z

src/ops/join.rs

+            .flat_map(|(left, rs)| {
+                rs.against
+                    .iter()
+                    .filter(move |&(right, _)| left < right)


Yeah, this is a somewhat annoying side-effect of using flat_map. move is perfectly acceptable to use here.

jonhoo · 2016-11-11T16:35:49Z

src/ops/join.rs

+                        let op = if rs.outer { "⋉" } else { "⋈" };
+                        rs.fields.iter().map(move |&(li, ri)| {
+                            format!("{}:{} {} {}:{}",
+                                left.index(), li, op, right.index(), ri)


This syntax reads weirdly to me. Can we come up with something that's easier to visually scan?

Do you have a suggestion? I picked node:col because it's consistent with unions: node:[col, col, col].

Ah, I meant more that I don't understand it. Is this showing which columns are emitted, or which columns are joined on? I feel like both should ideally be shown..

jonhoo · 2016-11-11T16:37:13Z

src/ops/latest.rs

+    fn description(&self) -> String {
+        let key_cols = self.key.iter().map(|k| k.to_string())
+            .collect::<Vec<_>>().join(", ");
+        format!("Latest γ[{}]", key_cols)


I'm tempted to have us use ⌈ ⌉ here, though its meaning would be somewhat overloaded.

This is consistent with the other grouping operators, though I guess latest isn't considered a grouping operator as far as the implementation is concerned. You tell me!

Oh, I'm fine with keeping the γ for grouping. Was more wondering if there's a more concise way to express "latest" rather than spelling it out.

How about the hourglass? ⧖

jonhoo · 2016-11-11T16:37:45Z

src/ops/mod.rs

@@ -315,7 +333,7 @@ impl Node {

 impl Debug for Node {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "{:?}", self.name)
+        write!(f, "{:?}({:#?})", self.name, *self.inner)


Why the *?

¯_(ツ)_/¯ that's just what it used to be: f2c8b42#diff-52cb7ae98bea74840c4725039169373cL318

Weird.. Does it compile without it?

jonhoo · 2016-11-11T16:38:17Z

src/ops/union.rs

+                format!("{}:[{}]", src.index(), cols)
+            })
+            .collect::<Vec<_>>()
+            .join(" ∪ ")


Shouldn't this be ⋃, not ∪ ?

benesch · 2016-11-11T20:27:38Z

Yeah, @jonhoo, the original PR added a write_dot method to the flow::Graph type instead of implementing Display. Happy to revert to that (possibly in its own trait) if you prefer. It'd be nice if Rust allowed you to install your own custom format codes

format!("{:dot}", graph)

impl FormatDot for Graph {
   fn fmt() { /* ... */ }
}

but alas.

ms705 · 2016-11-11T20:42:29Z

I think using Display and Debug in the way we do in the latest version is fine.

Adding a legend in Dot is a bit of a pain although it can be done (http://stackoverflow.com/a/4752766). I prefer "B" for "Base" over "F"/"Fact", but merely due to consistency with the implementation names.

jonhoo · 2016-11-11T21:41:42Z

Yeah, the ability to write custom formatting traits would be really neat. Maybe we should start an RFC for it :p

Output DOT graphs without using petgraph's stock formatter so that we can output each node's configuration using a dense, human-readable, relational-algebra-like format.

benesch · 2016-11-12T20:13:06Z

Here's the updated screenshot:

ms705 · 2016-11-12T20:15:57Z

@benesch Thanks, looks good!

Let's merge this -- it's a big improvement, and we have bigger research fish to fry 😉

benesch · 2016-11-12T20:17:35Z

Yessir ;)

benesch force-pushed the dot-graphs branch from da9958e to 19278a5 Compare November 10, 2016 23:06

ms705 approved these changes Nov 11, 2016

View reviewed changes

benesch force-pushed the dot-graphs branch 2 times, most recently from 8a8ace7 to 5a3c950 Compare November 11, 2016 05:57

jonhoo reviewed Nov 11, 2016

View reviewed changes

Produce even more readable DOT graphs

fef93ff

Output DOT graphs without using petgraph's stock formatter so that we can output each node's configuration using a dense, human-readable, relational-algebra-like format.

benesch force-pushed the dot-graphs branch from 5a3c950 to fef93ff Compare November 12, 2016 20:10

benesch merged commit 205616c into master Nov 12, 2016

benesch deleted the dot-graphs branch November 12, 2016 20:17

Produce even better DOT graphs #3

Produce even better DOT graphs #3

Conversation

benesch commented Nov 10, 2016

ms705 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesch commented Nov 11, 2016

jonhoo commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesch Nov 11, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesch commented Nov 11, 2016

ms705 commented Nov 11, 2016

jonhoo commented Nov 11, 2016

benesch commented Nov 12, 2016 • edited

ms705 commented Nov 12, 2016

benesch commented Nov 12, 2016

benesch Nov 11, 2016 •

edited

benesch commented Nov 12, 2016 •

edited