Add support for trace col grouping #101

tohrnii · 2022-12-10T08:54:14Z

Closes #83.

grjte

Thanks @tohrnii! The grammar looks good, but I think we may want to simplify the AST a bit to make life easier in the IR. See my comments inline and let me know if you have any questions

parser/src/ast/mod.rs

ir/src/symbol_table.rs

tohrnii · 2022-12-16T05:54:28Z

Thank you @grjte for the review, made suggested changes.

Al-Kindi-0 · 2022-12-16T06:22:47Z

parser/src/ast/mod.rs

+}
+
+/// [TraceCols] is used to represent a single or a group of columns in the execution trace. For
+/// single columns, the size is 0. For groups, the size is the number of columns in the group.


Just for my own understanding, is the number of columns sufficient to pinpoint the exact columns (e.g. contiguous and starting from 0)?

In response to @Al-Kindi-0:
Columns can be pinpointed by their index within a TraceCols struct. Keeping track of the size can be used for verification and also for having info required to output column groupings in codegen if needed. Internally in the IR, it's simplest to just think of things in terms of accessing a particular trace segment at a particular index and ignore groupings completely (e.g. for purposes of the constraint graph).

Regarding this doc comment:
Shouldn't the size be 1 for single columns? (i.e. the number of columns in the group)

@grjte Fixed.

@Al-Kindi-0's I'm not sure if you meant this but from your comment it seems like you mean whether column groups need to be contiguous? Currently they are assumed to be contiguous but I'm wondering if it's possible for some projects that it might be meaningful to have columns in a group that are not contiguous? (@grjte ?). I don't know if any clean syntax is possible to support such use cases though.

Thank you @grjte and @tohrnii , the answer of @grjte addresses my question. Regarding whether it might be useful to support non-contiguous cases, I can't think of cases where one might need it at the moment.

grjte

Thanks, looks good overall now. I left a few small things inline

ir/src/boundary_constraints.rs

ir/src/symbol_table.rs

ir/src/transition_constraints/graph.rs

parser/src/ast/mod.rs

grjte · 2022-12-16T19:55:36Z

parser/src/ast/mod.rs

+}
+
+/// [TraceCols] is used to represent a single or a group of columns in the execution trace. For
+/// single columns, the size is 0. For groups, the size is the number of columns in the group.


In response to @Al-Kindi-0:
Columns can be pinpointed by their index within a TraceCols struct. Keeping track of the size can be used for verification and also for having info required to output column groupings in codegen if needed. Internally in the IR, it's simplest to just think of things in terms of accessing a particular trace segment at a particular index and ignore groupings completely (e.g. for purposes of the constraint graph).

grjte · 2022-12-16T20:00:24Z

parser/src/ast/mod.rs

+}
+
+/// [TraceCols] is used to represent a single or a group of columns in the execution trace. For
+/// single columns, the size is 0. For groups, the size is the number of columns in the group.


Regarding this doc comment:
Shouldn't the size be 1 for single columns? (i.e. the number of columns in the group)

parser/src/ast/mod.rs

parser/src/parser/tests/trace_columns.rs

tohrnii · 2022-12-17T08:33:18Z

Thank you @grjte and @Al-Kindi-0 for the review. Made suggested changes.

grjte

Looks good to me, thanks for all your work @tohrnii!

tohrnii force-pushed the tohrnii-col-grouping-parser branch from cf79c39 to c62d27f Compare December 10, 2022 08:55

tohrnii requested review from bobbinth, Al-Kindi-0 and Fumuran and removed request for Fumuran, bobbinth and Al-Kindi-0 December 10, 2022 08:56

tohrnii marked this pull request as draft December 10, 2022 09:04

tohrnii changed the title ~~feat(parser): add support for trace col grouping~~ Add support for trace col grouping Dec 10, 2022

tohrnii force-pushed the tohrnii-col-grouping-parser branch from c5c7723 to 8197516 Compare December 10, 2022 10:15

tohrnii marked this pull request as ready for review December 10, 2022 10:16

tohrnii requested review from bobbinth, Al-Kindi-0 and Fumuran December 10, 2022 10:16

tohrnii mentioned this pull request Dec 10, 2022

Tracking PR for v0.2 release #79

Merged

14 tasks

grjte requested review from grjte and removed request for bobbinth December 14, 2022 12:06

grjte suggested changes Dec 15, 2022

View reviewed changes

parser/src/ast/mod.rs Outdated Show resolved Hide resolved

parser/src/ast/mod.rs Outdated Show resolved Hide resolved

ir/src/symbol_table.rs Outdated Show resolved Hide resolved

grjte mentioned this pull request Dec 15, 2022

Add trace column grouping to IR #102

Merged

tohrnii force-pushed the tohrnii-col-grouping-parser branch from 60f7512 to ae7f6ad Compare December 16, 2022 05:52

tohrnii force-pushed the tohrnii-col-grouping-parser branch from ae7f6ad to 0bb24c7 Compare December 16, 2022 05:55

tohrnii requested a review from grjte December 16, 2022 05:55

Al-Kindi-0 reviewed Dec 16, 2022

View reviewed changes

grjte suggested changes Dec 16, 2022

View reviewed changes

tohrnii force-pushed the tohrnii-col-grouping-parser branch from 0bb24c7 to a731396 Compare December 17, 2022 08:29

tohrnii requested review from grjte and Al-Kindi-0 December 20, 2022 14:21

grjte approved these changes Dec 21, 2022

View reviewed changes

tohrnii added 2 commits December 21, 2022 15:24

feat(parser): add support for trace col grouping to parser

e4df490

test: add trace col group tests and refactor old tests

e528374

tohrnii force-pushed the tohrnii-col-grouping-parser branch from a731396 to 1183f5d Compare December 21, 2022 15:30

refactor(parser): refactor trace columns AST

7b970b5

tohrnii force-pushed the tohrnii-col-grouping-parser branch from 1183f5d to 7b970b5 Compare December 21, 2022 15:32

grjte approved these changes Dec 21, 2022

View reviewed changes

Fumuran approved these changes Dec 22, 2022

View reviewed changes

Al-Kindi-0 approved these changes Dec 22, 2022

View reviewed changes

tohrnii merged commit 2209932 into next Dec 22, 2022

tohrnii deleted the tohrnii-col-grouping-parser branch December 22, 2022 14:22

tohrnii mentioned this pull request Dec 22, 2022

Add support for grouping of trace columns to parser #83

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for trace col grouping #101

Add support for trace col grouping #101

tohrnii commented Dec 10, 2022 •

edited

Loading

grjte left a comment

tohrnii commented Dec 16, 2022

Al-Kindi-0 Dec 16, 2022

grjte Dec 16, 2022

grjte Dec 16, 2022

tohrnii Dec 17, 2022

Al-Kindi-0 Dec 17, 2022

grjte left a comment

grjte Dec 16, 2022

grjte Dec 16, 2022

tohrnii commented Dec 17, 2022

grjte left a comment

Add support for trace col grouping #101

Add support for trace col grouping #101

Conversation

tohrnii commented Dec 10, 2022 • edited Loading

grjte left a comment

Choose a reason for hiding this comment

tohrnii commented Dec 16, 2022

Al-Kindi-0 Dec 16, 2022

Choose a reason for hiding this comment

grjte Dec 16, 2022

Choose a reason for hiding this comment

grjte Dec 16, 2022

Choose a reason for hiding this comment

tohrnii Dec 17, 2022

Choose a reason for hiding this comment

Al-Kindi-0 Dec 17, 2022

Choose a reason for hiding this comment

grjte left a comment

Choose a reason for hiding this comment

grjte Dec 16, 2022

Choose a reason for hiding this comment

grjte Dec 16, 2022

Choose a reason for hiding this comment

tohrnii commented Dec 17, 2022

grjte left a comment

Choose a reason for hiding this comment

tohrnii commented Dec 10, 2022 •

edited

Loading