set theory v2 #7534

Merged
merged 3 commits into from Aug 5, 2014

Conversation

Projects
None yet
7 participants
@bcoca
Member

bcoca commented May 25, 2014

Now it can handle non hashable items like dicts (which used to produce a TypeError)
Also it now makes each list unique before operating on them

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca May 25, 2014

Member

this should solve issues presented in #7495, without a need for a workaround

Member

bcoca commented May 25, 2014

this should solve issues presented in #7495, without a need for a workaround

@mpdehaan

This comment has been minimized.

Show comment
Hide comment
@mpdehaan

mpdehaan May 26, 2014

Contributor

Would it be possible to check the types of the arguments instead of the try/except?

Contributor

mpdehaan commented May 26, 2014

Would it be possible to check the types of the arguments instead of the try/except?

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca May 26, 2014

Member

I can check for dicts, but other types can be passed that are not
'hashable', the try/except covers all possible ones.

But considering that most of this should be comming from yaml, it should be
limited to dicts.

Brian Coca
Stultorum infinitus est numerus
0110000101110010011001010110111000100111011101000010000001111001011011110111010100100000011100110110110101100001011100100111010000100001
Pedo mellon a minno

Member

bcoca commented May 26, 2014

I can check for dicts, but other types can be passed that are not
'hashable', the try/except covers all possible ones.

But considering that most of this should be comming from yaml, it should be
limited to dicts.

Brian Coca
Stultorum infinitus est numerus
0110000101110010011001010110111000100111011101000010000001111001011011110111010100100000011100110110110101100001011100100111010000100001
Pedo mellon a minno

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca May 26, 2014

Member

actually, after thinking for 10s:

if isinstance(a, collections.Hashable) : ...

Member

bcoca commented May 26, 2014

actually, after thinking for 10s:

if isinstance(a, collections.Hashable) : ...

+ if isinstance(a,collections.Hashable) and isinstance(b,collections.Hashable):
+ c = set(a) | set(b)
+ else:
+ c = a + b

This comment has been minimized.

@kilburn

kilburn May 30, 2014

Contributor

Notice that, conceptually, the set(a) | set(b) operation is different than a + b. The former performs set union meaning that duplicates are removed, whereas the latter is a simple concatenation with the possibility for the same item to appear twice.

If these are considered "set operations", then the else part should be replaced by something like:

c = a + filter(lambda x: x not in a, b)
@kilburn

kilburn May 30, 2014

Contributor

Notice that, conceptually, the set(a) | set(b) operation is different than a + b. The former performs set union meaning that duplicates are removed, whereas the latter is a simple concatenation with the possibility for the same item to appear twice.

If these are considered "set operations", then the else part should be replaced by something like:

c = a + filter(lambda x: x not in a, b)
@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca May 30, 2014

Member

thanks, I had changed the rest, I'm not sure why I missed this one

Member

bcoca commented May 30, 2014

thanks, I had changed the rest, I'm not sure why I missed this one

@kilburn

This comment has been minimized.

Show comment
Hide comment
@kilburn

kilburn May 30, 2014

Contributor

Actually @mpdehaan pointed out that, by turning inputs into sets, we are removing duplicates in them too. Hence, the fallback operations are all wrong here.

Example with hashable items:

a,b = [x,x,y,y], [x,z,z]
unique(a) # set([x,y])
intersect(a, b) # set([x])
difference(a, b) # set([y])
symmetric_difference(a, b) # set([y,z])
union(a, b) # set([x,y,z])

Example with non-hashable items:

a,b = [x,x,y,y], [x,z,z]
unique(a) # set([x,x,y,y]) - Wrong: the not in c check is useless because c is populated only
                             after finishing the whole filter operation
intersect(a, b) # set([x,x]) - Wrong: duplicates in a are not removed
difference(a, b) # set([y,y]) - Wrong: duplicates in a are not removed
symmetric_difference(a, b) # set([y,y,z,z]) - Wrong: duplicates not removed from neither a nor b
union(a, b) # set([x,y,z]) # set([x,x,y,y,x,z,z]) - Wrong: duplicates not removed from neither a nor b

Also, the exact same result (than the set operations) is impossible to achieve because sets are implicitly unordered (meaning that you can get varying orders when iterating it) whereas lists are ordered by definition. Anyway, I guess this is an acceptable trade-off...

Contributor

kilburn commented May 30, 2014

Actually @mpdehaan pointed out that, by turning inputs into sets, we are removing duplicates in them too. Hence, the fallback operations are all wrong here.

Example with hashable items:

a,b = [x,x,y,y], [x,z,z]
unique(a) # set([x,y])
intersect(a, b) # set([x])
difference(a, b) # set([y])
symmetric_difference(a, b) # set([y,z])
union(a, b) # set([x,y,z])

Example with non-hashable items:

a,b = [x,x,y,y], [x,z,z]
unique(a) # set([x,x,y,y]) - Wrong: the not in c check is useless because c is populated only
                             after finishing the whole filter operation
intersect(a, b) # set([x,x]) - Wrong: duplicates in a are not removed
difference(a, b) # set([y,y]) - Wrong: duplicates in a are not removed
symmetric_difference(a, b) # set([y,y,z,z]) - Wrong: duplicates not removed from neither a nor b
union(a, b) # set([x,y,z]) # set([x,x,y,y,x,z,z]) - Wrong: duplicates not removed from neither a nor b

Also, the exact same result (than the set operations) is impossible to achieve because sets are implicitly unordered (meaning that you can get varying orders when iterating it) whereas lists are ordered by definition. Anyway, I guess this is an acceptable trade-off...

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca May 30, 2014

Member

I'll resubmit tonight, also making changes to try to coerce always
returning a list/set when used in templated fields (with_items:).​

Member

bcoca commented May 30, 2014

I'll resubmit tonight, also making changes to try to coerce always
returning a list/set when used in templated fields (with_items:).​

@mpdehaan mpdehaan added P4 and removed P3 labels Jun 1, 2014

@s0x

This comment has been minimized.

Show comment
Hide comment
@s0x

s0x Jul 16, 2014

Contributor

This even fixes an issue when using filters in with_items (e.g. #8055)

Contributor

s0x commented Jul 16, 2014

This even fixes an issue when using filters in with_items (e.g. #8055)

@srgvg

This comment has been minimized.

Show comment
Hide comment
@srgvg

srgvg Jul 16, 2014

Member

This patch fixes #8141 for me. +1

Member

srgvg commented Jul 16, 2014

This patch fixes #8141 for me. +1

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Jul 25, 2014

Member

+1 as we keep seeing people bitten by the use of non hashable objects with the set theory filters

Member

bcoca commented Jul 25, 2014

+1 as we keep seeing people bitten by the use of non hashable objects with the set theory filters

@jimi-c jimi-c merged commit ce8c8ab into ansible:devel Aug 5, 2014

@jimi-c

This comment has been minimized.

Show comment
Hide comment
@jimi-c

jimi-c Aug 5, 2014

Member

Merged, thanks!

Member

jimi-c commented Aug 5, 2014

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment