Skip to content

Scalding viz options#1440

Merged
MansurAshraf merged 9 commits intotwitter:developfrom
MansurAshraf:mashraf/scalding_viz
Sep 23, 2015
Merged

Scalding viz options#1440
MansurAshraf merged 9 commits intotwitter:developfrom
MansurAshraf:mashraf/scalding_viz

Conversation

@MansurAshraf
Copy link

Continuation of #1426 with published Chill version

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a Semigroup? Looks like a side-effecting function here. Are we guaranteed associativity?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think strategies will be applied in a sequence so it is associative (and not commutative)? /cc @ianoc as he wrote this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is a semigroup because plus is assocative

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not side-effecting. Nothing happens in plus. It is just creating a new strategy. Looks correct to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making an analogy to Haskell, it's equivalent to the semigroup over this type:

newtype FlowStepStrategy a =
    FlowStepStrategy (Flow a -> JList (FlowStep a) -> FlowStep a -> IO ())

instance Monoid (FlowStepStrategy a) where
    mempty = FlowStepStrategy (\_ _ _ -> return ())

    mappend (FlowStepStrategy f1) (FlowStepStrategy f2) = FlowStepStrategy f
      where
        f x y z = do
            f1 x y z
            f2 x y z

... and that's guaranteed to satisfy the Monoid laws in both Haskell and Scala.

The most common source of this thing failing in Scala specifically is if side effects are triggered prematurely by evaluating things in the wrong order but that does not appear to be the case for this code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok. What I was confused about was if plus(a, plus(b, c)).apply and plus(plus(a, b), c).apply will have the same effect.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why didn't you use the case style here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, use caused-by constructor (see below).

@johnynek
Copy link
Collaborator

Lgtm. What's up with the version number? What does exec mean here?

MansurAshraf pushed a commit that referenced this pull request Sep 23, 2015
@MansurAshraf MansurAshraf merged commit 2b29fce into twitter:develop Sep 23, 2015
@rubanm
Copy link
Contributor

rubanm commented Sep 23, 2015

+1

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 899b645 on MansurAshraf:mashraf/scalding_viz into ** on twitter:develop**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants