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

Bug/500 constant filters #501

Merged
merged 6 commits into from
Jul 10, 2024
Merged

Bug/500 constant filters #501

merged 6 commits into from
Jul 10, 2024

Conversation

aannleax
Copy link
Member

@aannleax aannleax commented Jul 4, 2024

I had code that computed the column on which a filter is going to be applied on. This depended upon the input variables, e.g. when computing a(x, y, z), 2 * x > z, the less-than filter should be applied on column z as the value for x has to be bound when computing the filter. The code that finds the last occurrence of a variable panicked if the filter did not contain any variable.

To fix this I simply assign constant filters to the first column.

Fixes #500.

@@ -125,7 +125,8 @@ impl GeneratorFilter {
}
}

unreachable!("Filter must only use markers from the table.")
// Compute constant columns in the first column
table[0]
Copy link
Member

Choose a reason for hiding this comment

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

From my high level understanding: Would it not be more intuitive to make this function return an Option?
Because in an example where the filter is only using constants, the column marker should really be none, no? (I could be wrong here, since I'm not totally sure what the OperationColumnMarker is.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the way the function is named, it makes sense. I changed it accordingly and set the default value after the call.

@mmarx mmarx added this to the Release 0.6.0 milestone Jul 5, 2024
@aannleax
Copy link
Member Author

aannleax commented Jul 5, 2024

Does anybody know a fix for the python binding?

The suggestion consider adding a #![type_length_limit="31589662"] attribute to your crate seems odd :D

@mmarx
Copy link
Member

mmarx commented Jul 5, 2024

The suggestion consider adding a #![type_length_limit="31589662"] attribute to your crate seems odd :D

Maybe that part of the parser could be rewritten to produce a smaller closure, but for now, I'd just go with increasing the limit. This is only coming up now because the check for this in rustc was broken for quite a while: rust-lang/rust#125507

Copy link
Collaborator

@matzemathics matzemathics left a comment

Choose a reason for hiding this comment

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

lgtm

@aannleax aannleax merged commit f55fe02 into main Jul 10, 2024
8 checks passed
@aannleax aannleax deleted the bug/500-constant-filters branch July 10, 2024 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Exception (unreachable code) when forgetting '?' in rule body
4 participants