Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

the Parser migration from fastparse to cats-parse #650

Closed
wants to merge 4 commits into from

Conversation

lvitaly
Copy link

@lvitaly lvitaly commented Nov 22, 2020

Relates to #631

@lvitaly lvitaly marked this pull request as draft November 22, 2020 16:37
@YulawOne
Copy link
Contributor

I think it worth to make some performance comparison between fastparse and cats-parse solution

@timzaak
Copy link
Contributor

timzaak commented Mar 19, 2021

If it's going on?

@ghostdogpr
Copy link
Owner

@timzaak I don't think there was much progress, in case you're interested to contribute ;)

@timzaak
Copy link
Contributor

timzaak commented Mar 23, 2021

I tried, but cats-parse api is now very different from fastparse. Almostly need rewrite all the code. and need wait this issue: typelevel/cats-parse#186
I will try to pass the Parser test cases. https://github.com/timzaak/caliban/tree/cat-parse

Honestly, It's may be a good idea to wait fastparse support Scala 3.

@lvitaly
Copy link
Author

lvitaly commented Mar 23, 2021

@timzaak @ghostdogpr I rewrote Parser with cats-parse but stuck with whitespace implementation. I will commit the last changes soon.

@timzaak
Copy link
Contributor

timzaak commented Mar 23, 2021

cats-parse 0.3.1 has changed a lot comparing with 0.1
you may update it firstly

@timzaak
Copy link
Contributor

timzaak commented Mar 23, 2021

@lvitaly I changed a lot depends on your work. remove fastparse implemention of whitespace, and try to insert it in everywhere needed. So Sad.

@timzaak
Copy link
Contributor

timzaak commented Mar 23, 2021

https://github.com/timzaak/caliban/blob/cat-parse/core/src/main/scala/caliban/parsing/Parser2.scala#L32 the whitespaces can work

@timzaak
Copy link
Contributor

timzaak commented Mar 28, 2021

@lvitaly @ghostdogpr
Parser2.scala now can pass all the test cases( except invalid syntax, becase cats-parse error is different from fastparse)

The performance would improve by handle whitespaces and trackback more carefully.

PS:

  1. string value implementation is from cats-parse JsonString.
  2. I firstly use cats-parse, so it needs code review before created merge.

@timzaak
Copy link
Contributor

timzaak commented Mar 28, 2021

@timzaak
Copy link
Contributor

timzaak commented Apr 3, 2021

I rewrite it, and it looks good to me, I don't know if it is good to replace fastparse, or just provide an alternative choice for GraphQL parse.

@ghostdogpr
Copy link
Owner

@timzaak one thing that would be interesting is to run the caliban benchmark project with fastparse vs cats-parse. If there is not much difference maybe we could migrate entirely. If cats-parse is much slower maybe we only adopt it temporarily inside a "scala3" branch.

@ghostdogpr
Copy link
Owner

Closing in favor of #850

@ghostdogpr ghostdogpr closed this May 2, 2021
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.

None yet

4 participants