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 #13045 - Comparison of Structs and Struct Exprs #13226
Conversation
return self._fields == other._fields \ | ||
if isinstance(other, Struct) \ | ||
else NotImplemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ed, looks great. Would you mind making the same fix for Interval, Locus, and Cal for consistency?
as you wish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
hail/python/hail/expr/functions.py
Outdated
wrapper['has_free_vars'] = ( | ||
x._ir.free_vars.__len__() > 0 or | ||
x._ir.free_agg_vars.__len__() > 0 or | ||
x._ir.free_scan_vars.__len__() > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a |=
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no difference for positive integers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean wrapper['has_free_vars'] |= ...
. Otherwise, if there are multiple nested expressions, and the last has no free variables but others do, we will only remember the False
from the last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes. Mutation. I forgot this can get called more than once
RR: #13045
RR: #13046
Support symmetric comparison of structs and struct expressions.
Provide better error messages when attempting to construct literals from expressions with free variables.