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

[User]Reader wraps SystemReader, not RawReader #567

Merged
merged 3 commits into from
Jun 14, 2023

Conversation

zslayton
Copy link
Contributor

@zslayton zslayton commented Jun 14, 2023

The UserReader (alias: Reader) is implemented as a (RawReader, SymbolTable) pair. If/when the RawReader encounters a symbol table, the Reader consumes it and adds its symbols to the SymbolTable.

The SystemReader is a very similar; it is also a (RawReader, SymbolTable) pair. However, when the RawReader encounters a symbol table, the SystemReader cannot consume it. Instead, it needs to process only the value over which the RawReader is currently positioned. This requires some complex state tracking, but it allows the SystemReader to expose/return system-level values while also processing them.

This PR modifies the UserReader to be a thin wrapper around the SystemReader. Whenever the SystemReader reports one or more system-level values, the UserReader skips over them until it finds the next user-level value or exhausts the input stream. This change is in commit 727417f.

When the SystemReader was originally written, implementations of the RawReader were not required to have idempotent read_* operations; they tended to consume the value when it was read, meaning applications had to materialize/store the value if they wished to refer to it multiple times. Given this constraint, when the SystemReader encountered part of a symbol table, it had to materialize it so it could be processed once by the SystemReader and (potentially) a second time by the user.

This constraint no longer exists; all RawReader implementations now allow a user to call read_* any number of times, always returning the same value. This PR also removes the machinery that was needed to store a materialized copy of system values; it does this in commit e6c3b87.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@zslayton zslayton requested a review from desaikd June 14, 2023 20:27
@zslayton zslayton marked this pull request as ready for review June 14, 2023 20:28
Copy link
Contributor

@desaikd desaikd left a comment

Choose a reason for hiding this comment

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

Looks good!

@zslayton zslayton merged commit 82f17fc into main Jun 14, 2023
20 of 21 checks passed
@zslayton zslayton deleted the user-reader-wraps-system-reader branch June 14, 2023 20:59
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

2 participants