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

Subtype: Improve simple_meet resolution for Union inputs #49376

Merged
merged 3 commits into from
Apr 22, 2023

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Apr 16, 2023

close #49354. (The fix is somehow limited as obviously_disjoint is simple to deceive.)

@N5N3 N5N3 added the types and dispatch Types, subtyping and method dispatch label Apr 16, 2023
@N5N3 N5N3 requested a review from vtjnash April 16, 2023 16:43
@giordano
Copy link
Contributor

To be backported to 1.9?

@KristofferC KristofferC added the backport 1.9 Change should be backported to release-1.9 label Apr 16, 2023
src/jltypes.c Outdated
}
if (subab && !subba) {
stemp[j] = -1;
temp[j] = temp[i];
Copy link
Member

@vtjnash vtjnash Apr 17, 2023

Choose a reason for hiding this comment

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

Does this make the algorithm slightly unstable to ordering? Eventually, for !overesi, this algorithm could return Union{a, b} for all a and b from the original a and b for which it could prove that element is a <: of the other union. That may often be Union{}, but doesn't have to be.

Secondly for overesi, it similarly can return any type that is both <:a and <:b but >:intersect(a,b). I don't think that is quite the same as removing any element from b that is potentially a subtype of an element a. For example, if it had Tuple{E1} and Tuple{E2} in a, but had Tuple{Union{E1,E2}} in b, it may be incorrect to "forget" that Tuple{E2} was in the union (unless !overesi)

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'm afraid that I didn’t get the 1st concern, simple_intersectonly joins preserved types in a, or preserved and substituted ones inb, thus it seems unlikely to return Union{a,b}?

But the 2nd concern is correct, the current algorithm works well only for injective elements. We’d better perform the “substitution” after the subtyping

Copy link
Member

Choose a reason for hiding this comment

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

For !overesi, it isn't a concern, but an observation that this can return the Union of all provably preservable elements (all elements of a that are definite subtypes of b and all elements of b that are definitely subtypes of a)

Copy link
Member Author

@N5N3 N5N3 Apr 18, 2023

Choose a reason for hiding this comment

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

IIUC, this is exactly what we want?
The result should >: all elements from a if it <:b and all elements from b if it <:a.
I think there's no need to worry about Union{Int, Real} ∩ Integer (a[i] <: b[j] <: a[k]) as all other path should have normalized it.

Copy link
Member

Choose a reason for hiding this comment

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

It may already be doing much of this, and I just haven't fully understood that part of the algorithm. The example I am thinking here is that if you had a=Union{Int32, Int64, AbstractString} and b=Union{Integer, String}, then the !overesi intersection can be any type that is <:Union{Int32, Int64, String} (often chosen as Union{} currently) while the overesi intersection is any value that is >:Union{Int32, Int64, String}, which might be chosen as either of a or b currently.

Copy link
Member Author

@N5N3 N5N3 Apr 18, 2023

Choose a reason for hiding this comment

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

For !overesi, the algorithm requires all undisjointable part in a(b) <: b(a), otherwise it would always return Union{}.
Since the "Subtitution" has been replaced with an elimination with partiality, elements in a would be removed if we can't prove it <:ₛb, while elements in b would be removed only if we can prove it >:ₛa. ( means strick)

src/jltypes.c Outdated Show resolved Hide resolved
@N5N3 N5N3 force-pushed the subtype-tuning branch 2 times, most recently from 8156ee8 to 249e819 Compare April 18, 2023 14:55
N5N3 added 2 commits April 18, 2023 22:56
ans make sure the duplicated elements in `a` get removed
@N5N3 N5N3 force-pushed the subtype-tuning branch 2 times, most recently from 249e819 to c9d1ab2 Compare April 18, 2023 15:03
This should be fast, as it has an `obviously_disjoint`-based fast path.
@N5N3 N5N3 merged commit 6b79e8c into JuliaLang:master Apr 22, 2023
@N5N3 N5N3 deleted the subtype-tuning branch April 22, 2023 16:59
N5N3 added a commit that referenced this pull request Apr 22, 2023
* Improve `simple_meet` resolution.

* Fix for many-to-one cases.

* Test disjoint via `jl_has_empty_intersection`
@N5N3 N5N3 mentioned this pull request Apr 22, 2023
26 tasks
@N5N3 N5N3 removed the backport 1.9 Change should be backported to release-1.9 label Apr 22, 2023
@timholy
Copy link
Member

timholy commented Apr 22, 2023

Thanks @N5N3!

@KristofferC
Copy link
Member

@N5N3, this seems to fail analyzegc on the 1.9 backport branch.

@N5N3
Copy link
Member Author

N5N3 commented Apr 24, 2023

Sorry, didn’t notice that. Looks like there’re some GC mark change on master.
Will take a look tomorrow.

N5N3 added a commit to N5N3/julia that referenced this pull request Apr 24, 2023
…ng#49376)

* Improve `simple_meet` resolution.

* Fix for many-to-one cases.

* Test disjoint via `jl_has_empty_intersection`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Testing Arrow asserts in subtyping on 1.9
5 participants