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
Support casting between scalars with a common concrete base #5108
Conversation
Seems like this was not supported because of an oversight, and then I unthinkingly added a test that asserted the behavior after I permuted some related code in #3662.
7d1a6ce
to
94eccd7
Compare
edb/edgeql/compiler/casts.py
Outdated
and (nearest := s_utils.get_class_nearest_common_ancestors( | ||
ctx.env.schema, [new_stype, orig_stype])) | ||
and len(nearest) == 1 | ||
and not nearest[0].get_abstract(ctx.env.schema) |
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 wouldn't work if the nearest common ancestor is an abstract subtype of a non-abstract parent, e.g
create abstract scalar type foo extending str;
create scalar type bar1 extending foo;
create scalar type bar2 extending foo;
So:
and not nearest[0].get_abstract(ctx.env.schema) | |
and nearest[0].maybe_get_topmost_concrete_base(ctx.env.schema) is not None |
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.
Good catch
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.
Actually, I think the whole check should just compare the results of maybe_get_topmost_concrete_base
, because
create abstract scalar type foo extending str;
create abstract scalar type bar extending str;
create scalar type bar1 extending foo, bar;
create scalar type bar2 extending foo, bar;
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.
Oh, yeah, indeed; I had misremembered and thought scalars could only have one base, but it's actually they can have one concrete ancestor
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.
And this is probably faster too
Seems like this was not supported because of an oversight, and then I
unthinkingly added a test that asserted the behavior after I permuted
some related code in #3662.