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.

Sign up for GitHub

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

Jump to bottom

Parser Step representation: make ParserK and ParserD consistent #2433

Open
adithyaov opened this issue Jul 14, 2023 · 5 comments · May be fixed by #2811
Open

Parser Step representation: make ParserK and ParserD consistent #2433

adithyaov opened this issue Jul 14, 2023 · 5 comments · May be fixed by #2811
Assignees
@adithyaov
Labels
api:breaking Incompatible with existing releases aspect:parsers priority:high
Milestone
0.11.0

Comments

@adithyaov
Copy link
Member

In ParserD the Int in step indicates the amount of elements to backtrack.

  • Partial 3 a means go back by 3 elements before running the driver again.
  • Partial 0 a run the driver without going back.

In ParserK the Int in step indicates the relative stream position to be in.
Partial (+3) a means go forward by 3 elements and run the driver. (Unimplemented)
Partial 1 a means go forward by 3 elements and run the driver.
Partial 0 a means run the driver over the current element again.
Partial (-3) a means go back by 3 elements and run the driver.

We need to unify the representation.
The question is which representation makes more sense.

ParserK representation is much better as it represents even the forward seek as well.
Even if not for forward seek, it's more expressive in the sense that there is no implicit meaning of driver going forward. We explicitly control it by saying Partial 1.
@adithyaov adithyaov changed the title Parser Step representation Parser Step representation Jul 14, 2023
@harendra-kumar harendra-kumar added this to the 0.11.0 milestone Jan 28, 2024
@harendra-kumar harendra-kumar added the api:breaking Incompatible with existing releases label Jan 28, 2024
@harendra-kumar
Copy link
Member

Let's change this in 0.11 to adopt the ParserK implementation in Parser as well. It is intuitive, it is required in ParserK for chunk parsing, it will enable chunk parsing in Parser as well, we can also use it for SIMD implementation for folds/parsers.

Need to change all existing parsers. Users who are using the Parser constructor to create custom parsers will be affected by the change. Need to update the changelog accordingly. The change required is - in Partial n, Continue n, Done n n should be changed to 1-n.

@harendra-kumar
Copy link
Member

We can deprecate the internal parser module, anyone using custom parsers would be using the internal module because Parser constructor is not exposed from the released module.

@harendra-kumar harendra-kumar mentioned this issue Jul 20, 2024
Closed
@harendra-kumar
Copy link
Member

See also #2459 .

@harendra-kumar harendra-kumar changed the title Parser Step representation Parser Step representation: make ParserK and ParserD consistent Jul 20, 2024
@adithyaov
Copy link
Member Author

There is a PR for this. After the soak-in time this can be discussed further.

@adithyaov
Copy link
Member Author

Rebase the PR

@adithyaov adithyaov linked a pull request Oct 4, 2024 that will close this issue
Make ParserD consistent with ParserK #2811
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adithyaov adithyaov

Labels
api:breaking Incompatible with existing releases aspect:parsers priority:high
Projects
None yet
Milestone
0.11.0
Development

Successfully merging a pull request may close this issue.

Make ParserD consistent with ParserK composewell/streamly
2 participants
@harendra-kumar @adithyaov