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

Fix operation batch flags constraints #1495

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Sep 13, 2024

Fixes the operation batch flag constraints, as well as a minor fix in the op group table auxiliary constraints.

@plafer plafer added the documentation Improvements or additions to documentation label Sep 13, 2024
@plafer plafer added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Sep 13, 2024
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Great fixes! Thank you! I left one comment inline.

docs/src/design/decoder/constraints.md Outdated Show resolved Hide resolved
@bobbinth
Copy link
Contributor

One simplification idea: we could get rid of op_batch flags entirely by always adding 8 operation groups to the op_group table. The cost of this is that for empty groups we'd need to execute a single noop operation (so, in the worst case, 7 noops in total). But it will make the constraint system simpler and will allow us to get rid of 3 columns (I think).

@bobbinth
Copy link
Contributor

bobbinth commented Sep 13, 2024

Or maybe there is a half-way solution where we have a single flag which basically says "add either 4 or 8" groups. In this case, the constraints are also quite a bit simpler, we get rid of 2 columns, and the worst case cost is 3 extra noops per span.

Copy link
Collaborator

@Al-Kindi-0 Al-Kindi-0 left a comment

Choose a reason for hiding this comment

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

Great catch!
The only thing I think that needs to change is the constraint which should be separated in order to guarantee the "all" part.

Copy link
Contributor

@bobbinth bobbinth 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! Thank you!

Copy link
Collaborator

@Al-Kindi-0 Al-Kindi-0 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 to me.

@plafer plafer merged commit f06923b into next Sep 17, 2024
9 checks passed
@plafer plafer deleted the plafer-fix-decoder-constraints branch September 17, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation no changelog This PR does not require an entry in the `CHANGELOG.md` file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants