filter, based on selective #101

rossabaker · 2020-12-06T07:46:39Z

Riffing on the ideas of Staged Selective Parser Combinators.

Use case: in http4s, we need to parse various date headers into a 7-tuple of (weekday, year, month, day, hour, minute, second), and then verify that the date is valid (e.g., no 31st of February). Currently we have to flatMap it, and when parsing, we don't want to flatMap that shit.

Implemented on branch. We should have the full power of Selective. The primitive operation could also be select, which is the abstract function on cats-selective.

Should be able to make a version that fails with a particular message, swapping that in for "empty".

There may be optimizations to sprinkle throughout the code. Fusing to Impl.Map seems possible, for instance.

I'm super sleepy and just following types. This might all be nonsense.

johnynek

This is really cool. I think we should add it.

One note: users can implement filter, with less performance, using flatMap in the mean time (I guess any time you have Monad + Alternative this is true).

I had a couple concerns that I noted.

core/shared/src/main/scala/cats/parse/Parser.scala

johnynek · 2020-12-06T17:03:57Z

core/shared/src/test/scala/cats/parse/ParserTest.scala

+      val res = genP.fa.filter(_ => false).parse(str)
+      assert(res.isLeft)
+    }
+  }


Can we write a law on branch directly (see the laws I have on how ~ composes). I don't think what we have now is exactly right but since filter is weaker is might not detect it.

johnynek · 2020-12-06T17:05:55Z

core/shared/src/main/scala/cats/parse/Parser.scala

+      val either = parser.parseMut(state)
+      if ((state.error eq null) && state.capture)
+        either.fold(a => l.ap(Parser.pure(a)), b => r.ap(Parser.pure(b))).parseMut(state)
+      else null.asInstanceOf[C]


I think this isn't right. I think if we are not capturing we still need to run the result because it may advance the offset in the parser.

core/shared/src/test/scala/cats/parse/ParserTest.scala

rossabaker · 2020-12-06T17:11:05Z

Ugh. I didn't think anybody had looked at it yet, and just force-pushed it based on select and fixing some 3am mistakes. But I think a lot of your feedback is still valid.

I'd also like to throw in an example and perhaps a bench to show how it compares to the monadic approach.

codecov-io · 2020-12-06T17:13:08Z

Codecov Report

Merging #101 (3ae0d63) into main (a144809) will decrease coverage by 0.27%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##             main     #101      +/-   ##
==========================================
- Coverage   96.44%   96.17%   -0.28%     
==========================================
  Files           5        6       +1     
  Lines         676      732      +56     
  Branches       57       68      +11     
==========================================
+ Hits          652      704      +52     
- Misses         24       28       +4

Impacted Files	Coverage Δ
core/shared/src/main/scala/cats/parse/Parser.scala	`96.94% <93.75%> (-0.35%)`	⬇️
...shared/src/main/scala/cats/parse/LocationMap.scala	`95.83% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a144809...3ae0d63. Read the comment docs.

johnynek · 2020-12-06T17:30:36Z

Dude I check my GitHub notifications religiously. Well really more than most people do religious things.

johnynek · 2020-12-06T17:45:16Z

core/shared/src/main/scala/cats/parse/Parser.scala

+        fn: Parser[A => B],
+        state: State
+    ): B = {
+      val either = parser.parseMut(state)


Actually, look at flatMap. I think you have to set capture to true here unconditionally since otherwise we don't know what to do next.

After we capture the first, we can set capture to false on the second part since we can just avoid applying the function.

johnynek · 2020-12-06T17:47:05Z

core/shared/src/main/scala/cats/parse/Parser.scala

@@ -911,6 +914,9 @@ object Parser extends ParserInstances {
      case _ => Impl.Map1(p, fn)
    }

+  def select[A, B](p: Parser[Either[A, B]])(fn: Parser[A => B]): Parser[B] =


For bonus points we can add select10 that takes a Parser1 then a Parser and returns a Parser1. Similarly a select01 that does the Parser1 second.

If we had that, we can override filter on Parser1 to return a Parser1

Actually with this formulation I think we only need select and select1. In select1 the first argument is a Parser1.

johnynek · 2020-12-06T17:48:41Z

core/shared/src/test/scala/cats/parse/ParserTest.scala

+      assertEquals(res0, res1)
+    }
+  }
+


Another interesting pair of laws: if we pa.ap(pb) should be the same as select(pa.map(Left), pb, fail)

Similarly on the right hand side.

johnynek · 2020-12-06T19:18:11Z

core/shared/src/main/scala/cats/parse/Parser.scala

@@ -1331,6 +1337,9 @@ object Parser extends ParserInstances {
        case Map(p, _) =>
          // we discard any allocations done by fn
          unmap(p)
+        case Select(p, _) =>
+          // we discard any allocations done by fn
+          unmap(p)


I think you need to do Select(a, b) => Select(a, unmap(b))

Since in select, after we optionally parse the right in a void we could not do anything more, but we do need the result on the left even in a void.

The second parameter has to be a function, so I had to immediately and trivially map the unmapped value.

core/shared/src/test/scala/cats/parse/ParserTest.scala

johnynek · 2020-12-07T02:54:12Z

core/shared/src/main/scala/cats/parse/Parser.scala

+      state.capture = cap
+      if (state.error eq null)
+        either match {
+          case Left(a) => fn.map(_(a)).parseMut(state)


I think you can do fun.parseMut(state) and in the capture case actually apply the function. In the non capture case you can just return null.

I think this also solves the unmap requirement of putting in const.

The .asInstanceOf in the unmap makes me queasy, but it makes the existing tests pass.

rossabaker · 2020-12-07T05:02:25Z

core/shared/src/test/scala/cats/parse/ParserTest.scala

+  property("select(pa.map(Left(_)))(pf) == pf.ap(pa)") {
+    forAll(ParserGen.gen, ParserGen.gen) { (genP, genRes) =>
+      val pa = genP.fa
+      val pf = null: Parser[genP.A => genRes.A]


I'm struggling to produce this parser. The trick used in the generator didn't quite translate.

johnynek · 2020-12-07T22:20:56Z

I made a PR as a suggestion to this one:

rossabaker#1

It gets everything green, but changes somewhat the internal representation of Select (to work more cleanly with unmap).

some suggestions for selective

johnynek

What do you think about merging this and publishing 0.2.0?

I think we need to remove the "Draft" label.

rossabaker · 2020-12-08T03:26:35Z

I'm good with that. I'd still like to figure out how to write that Left law, and I'm also interested in a benchmark that shows the advantage (or not) over flatMap. But none of that needs to hold back progress.

mpilquist · 2020-12-08T03:27:48Z

Bump ThisBuild / baseVersion before doing so -- not sure that failing to do so will cause a problem but should do it anyway.

johnynek · 2020-12-08T03:32:44Z

Actually I think adding an override for ifM using select:

https://github.com/typelevel/cats/blob/8e8be51187385dde78321e7e0d9fd892801afacb/core/src/main/scala/cats/FlatMap.scala#L123

We should be able but I don't see it immediately. I assume we use select twice.

rossabaker · 2020-12-08T03:34:22Z

baseVersion is already bumped.

Devil's advocacy: a risk in merging this before a benchmark is that the gains may be only theoretical and not justify the new primitive. I definitely want filter, but there's also a monadic way to express it.

That said, I think it is better, and even if it's not, it's still 0.x.

johnynek · 2020-12-08T03:35:42Z

I bet ifM will be a lot faster using select (or a similar internal node).

rossabaker · 2020-12-08T03:40:39Z

cats-selective and its Haskell forebears call it ifS, and we'd get it for free with a cats-selective integration.

If this proves successful, I'd be interested in renewing the push to get Selective into Cats proper. With the right encoding, I think it could even be accepted into 2.x.

regadas · 2020-12-08T23:26:37Z

This is really cool! 🎉

filter, based on selective functors

64e0c2a

rossabaker force-pushed the selective branch from fbf57a1 to 64e0c2a Compare December 6, 2020 17:07

johnynek requested changes Dec 6, 2020

View reviewed changes

rossabaker added 2 commits December 6, 2020 12:18

Replace valueOr with a pattern match

5cea231

Missed a scalafmt

07e35af

johnynek reviewed Dec 6, 2020

View reviewed changes

johnynek mentioned this pull request Dec 6, 2020

Fix return type on repAs1 #99

Merged

Rudimentary Select generator

3bfdef7

rossabaker commented Dec 6, 2020

View reviewed changes

core/shared/src/test/scala/cats/parse/ParserTest.scala Outdated Show resolved Hide resolved

core/shared/src/test/scala/cats/parse/ParserTest.scala Outdated Show resolved Hide resolved

core/shared/src/test/scala/cats/parse/ParserTest.scala Outdated Show resolved Hide resolved

rossabaker added 2 commits December 6, 2020 20:34

More sophisticated Select generator

c060091

Passing tests

273e769

johnynek reviewed Dec 7, 2020

View reviewed changes

rossabaker added 2 commits December 6, 2020 22:19

Conditionally apply the selective function

d8f62ed

select laws: not compiling yet

1de3a71

rossabaker commented Dec 7, 2020

View reviewed changes

some suggestions for selective

546e0af

rossabaker added 2 commits December 7, 2020 20:16

Update core/shared/src/main/scala/cats/parse/Parser.scala

04d030a

Merge pull request #1 from typelevel/oscar/selective_suggestions

3ae0d63

some suggestions for selective

johnynek approved these changes Dec 8, 2020

View reviewed changes

rossabaker marked this pull request as ready for review December 8, 2020 03:26

rossabaker mentioned this pull request Dec 8, 2020

add Selective typelevel/cats#2745

Open

johnynek merged commit a7fb824 into typelevel:main Dec 8, 2020

rossabaker deleted the selective branch December 9, 2020 04:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filter, based on selective #101

filter, based on selective #101

rossabaker commented Dec 6, 2020

johnynek left a comment

johnynek Dec 6, 2020

johnynek Dec 6, 2020

rossabaker commented Dec 6, 2020

codecov-io commented Dec 6, 2020 •

edited

Loading

johnynek commented Dec 6, 2020

johnynek Dec 6, 2020

johnynek Dec 6, 2020

johnynek Dec 7, 2020

johnynek Dec 6, 2020

johnynek Dec 6, 2020

rossabaker Dec 7, 2020

johnynek Dec 7, 2020

rossabaker Dec 7, 2020

rossabaker Dec 7, 2020

johnynek commented Dec 7, 2020

johnynek left a comment

rossabaker commented Dec 8, 2020

mpilquist commented Dec 8, 2020

johnynek commented Dec 8, 2020

rossabaker commented Dec 8, 2020

johnynek commented Dec 8, 2020

rossabaker commented Dec 8, 2020 •

edited

Loading

regadas commented Dec 8, 2020

filter, based on selective #101

filter, based on selective #101

Conversation

rossabaker commented Dec 6, 2020

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossabaker commented Dec 6, 2020

codecov-io commented Dec 6, 2020 • edited Loading

Codecov Report

johnynek commented Dec 6, 2020

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

johnynek commented Dec 7, 2020

johnynek left a comment

Choose a reason for hiding this comment

rossabaker commented Dec 8, 2020

mpilquist commented Dec 8, 2020

johnynek commented Dec 8, 2020

rossabaker commented Dec 8, 2020

johnynek commented Dec 8, 2020

rossabaker commented Dec 8, 2020 • edited Loading

regadas commented Dec 8, 2020

codecov-io commented Dec 6, 2020 •

edited

Loading

rossabaker commented Dec 8, 2020 •

edited

Loading