Groovy
  1. Groovy
  2. GROOVY-2904

[ [a:1, b:2, c:3] ].flatten() -> [ 1, 2, 3 ], but should be [ [a:1, b:2, c:3] ]

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6-beta-1
    • Fix Version/s: 1.6-rc-2, 1.5.8, 1.7-beta-1
    • Component/s: None
    • Labels:
      None
    • Environment:
      Patch relative to SVN revision 12775.
    • Flags:
      Patch

      Description

      Updated list#flatten/set#flatten so that maps are unadulterated.

      Going with this approach means that maps can safely be used in lists. This is commonly done as a kind of "pseudobean" for unit testing – it's how I first encountered this behavior.

        Issue Links

          Activity

          Hide
          Robert Fischer added a comment -
          Show
          Robert Fischer added a comment - See GROOVY-2903 ( http://jira.codehaus.org/browse/GROOVY-2903 ) for more on this issue.
          Hide
          Paul King added a comment -

          Alternative patch

          Show
          Paul King added a comment - Alternative patch
          Hide
          Robert Fischer added a comment -

          I'd suggest that flatten-with-closure get moved into its own ticket – it's an interesting idea, but really tangential to issue being considered here. #flatten specifies that it recursively adds Collections, which is the most straightforward and easily conceptualized approach, so that's what my patch did. We should at least fix the code to adhere to its contract, and then we can discuss flatten-with-closure syntax and behavior.

          The problem with recursively adding map content (or the superset of that functionality, recursively adding any iterable thing in the list), is that it's really going to wreak havoc on unit tests. If you "flatten" maps in some way, this means that you're getting different behavior if you use a bean or a hash map in its place: this is exceedingly common in unit testing, including making it onto the website as a recommended pattern.

          Any patch which pushes the "flatten maps" approach also needs to include a patch to create a map#flatten which has parallel behavior.

          Show
          Robert Fischer added a comment - I'd suggest that flatten-with-closure get moved into its own ticket – it's an interesting idea, but really tangential to issue being considered here. #flatten specifies that it recursively adds Collections, which is the most straightforward and easily conceptualized approach, so that's what my patch did. We should at least fix the code to adhere to its contract, and then we can discuss flatten-with-closure syntax and behavior. The problem with recursively adding map content (or the superset of that functionality, recursively adding any iterable thing in the list), is that it's really going to wreak havoc on unit tests. If you "flatten" maps in some way, this means that you're getting different behavior if you use a bean or a hash map in its place: this is exceedingly common in unit testing, including making it onto the website as a recommended pattern. Any patch which pushes the "flatten maps" approach also needs to include a patch to create a map#flatten which has parallel behavior.
          Hide
          Paul King added a comment -

          When you say "which has parallel behavior", what did you mean exactly?

          Show
          Paul King added a comment - When you say "which has parallel behavior", what did you mean exactly?
          Hide
          Robert Fischer added a comment -

          If a list containing a map somehow mangles that map, then map#flatten should mangle the map in the same way. This will retain the "recursive" flavor of #flatten.

          Show
          Robert Fischer added a comment - If a list containing a map somehow mangles that map, then map#flatten should mangle the map in the same way. This will retain the "recursive" flavor of #flatten.
          Hide
          Paul King added a comment -

          altered patch for Collection supporting arrays; defaults to no flattening of maps

          Show
          Paul King added a comment - altered patch for Collection supporting arrays; defaults to no flattening of maps
          Hide
          James P. White added a comment -

          This patch isn't quite correct. Whatever you're going to do to the children needs to be symmetric wrt to what you do at the top-level.

          If you're going to flatten arrays, then (Object[]).flatten() needs to work too. If you're gonna do that you have to implement DGM.flatten(Object) and DGM.flatten(Object, Closure).

          Show
          James P. White added a comment - This patch isn't quite correct. Whatever you're going to do to the children needs to be symmetric wrt to what you do at the top-level. If you're going to flatten arrays, then (Object[]).flatten() needs to work too. If you're gonna do that you have to implement DGM.flatten(Object) and DGM.flatten(Object, Closure).
          Hide
          Jochen Theodorou added a comment - - edited

          yes, or else int[][][] for example could not be "flattened".. which makes me think, that we maybe should not flatten arrays

          Show
          Jochen Theodorou added a comment - - edited yes, or else int[][][] for example could not be "flattened".. which makes me think, that we maybe should not flatten arrays
          Hide
          Robert Fischer added a comment -

          What's wrong with flattening arrays?

          Show
          Robert Fischer added a comment - What's wrong with flattening arrays?
          Hide
          Jochen Theodorou added a comment -

          nothing, it was just a thought. If we allow flatten for any array, then a DGM method won't do it. I then suggest that we handle it like we do the size() method on arrays.. by special code from inside the MetaClass. Of course then flatten won't show up in the API

          Show
          Jochen Theodorou added a comment - nothing, it was just a thought. If we allow flatten for any array, then a DGM method won't do it. I then suggest that we handle it like we do the size() method on arrays.. by special code from inside the MetaClass. Of course then flatten won't show up in the API
          Hide
          Paul King added a comment -

          Patch for Collection applied. I am still not sure I see the value in doing this for arrays.

          Show
          Paul King added a comment - Patch for Collection applied. I am still not sure I see the value in doing this for arrays.
          Hide
          Paul King added a comment -

          Can someone who wants array flattening please provide some use cases, otherwise I think I will close this issue. The current patch would provide value in that you could convert an array to a Collection then flatten it then convert back.

          Show
          Paul King added a comment - Can someone who wants array flattening please provide some use cases, otherwise I think I will close this issue. The current patch would provide value in that you could convert an array to a Collection then flatten it then convert back.
          Hide
          Paul King added a comment -

          Implemented for Collections. Please reopen if you have some use cases not covered.

          Show
          Paul King added a comment - Implemented for Collections. Please reopen if you have some use cases not covered.
          Hide
          James P. White added a comment - - edited

          I really can't see any justification for flattening arrays which are elements of a collection but not allowing arrays to be flattened. It should be one way or the other.

          This:

          [[[1, 2, 3, [1, 2, 3, [1, 2, 3] as Object[]] as Object[]] as Object[]] as Object[]].flatten()
          ==>
          [1, 2, 3, 1, 2, 3, 1, 2, 3]
          

          is far too inconsistent with this:

          ([1, 2, 3] as Integer[]).flatten()
          ==>
          groovy.lang.MissingMethodException: No signature of method: [Ljava.lang.Integer;.flatten() is applicable for argument types: () values: {}
          

          A significant case on why that is important is that a user wondering whether arrays are flattened or not will try the simple (second) case first and conclude that arrays are not flattened. They will be greatly and not pleasantly surprised when they discover that is just a special case and that Groovy does flatten arrays, but only when the top-level is a Collection.

          Show
          James P. White added a comment - - edited I really can't see any justification for flattening arrays which are elements of a collection but not allowing arrays to be flattened. It should be one way or the other. This: [[[1, 2, 3, [1, 2, 3, [1, 2, 3] as Object []] as Object []] as Object []] as Object []].flatten() ==> [1, 2, 3, 1, 2, 3, 1, 2, 3] is far too inconsistent with this: ([1, 2, 3] as Integer []).flatten() ==> groovy.lang.MissingMethodException: No signature of method: [Ljava.lang. Integer ;.flatten() is applicable for argument types: () values: {} A significant case on why that is important is that a user wondering whether arrays are flattened or not will try the simple (second) case first and conclude that arrays are not flattened. They will be greatly and not pleasantly surprised when they discover that is just a special case and that Groovy does flatten arrays, but only when the top-level is a Collection.
          Hide
          Paul King added a comment - - edited

          It is an interesting edge case. What would you expected ([1, 2, 3] as Integer[]).flatten() to return? Just the original elements but now in a List? Normally flatten() is trying to preserve the original type. If we preserve the original type, we could just use the unity function except for Object[] which would require further processing and perhaps multi-dimensional arrays going to single (if we aren't expecting the multi-dimensional nature of the original array to be preserved).

          Show
          Paul King added a comment - - edited It is an interesting edge case. What would you expected ( [1, 2, 3] as Integer[]).flatten() to return? Just the original elements but now in a List ? Normally flatten() is trying to preserve the original type. If we preserve the original type, we could just use the unity function except for Object[] which would require further processing and perhaps multi-dimensional arrays going to single (if we aren't expecting the multi-dimensional nature of the original array to be preserved).
          Hide
          James P. White added a comment - - edited

          I wouldn't expect any different btw ([1, 2, 3] as Integer[]).flatten() and ([1, 2, 3] as Object[]).flatten(), just as there is no difference btw ([[1, 2, 3] as Integer[]]).flatten() and ([[1, 2, 3] as Object[]]).flatten().

          So yes, the result is a List, which is what createSimilarOrDefaultCollection creates.

          And flatten as it is currently implemented is flattening multidimensional arrays (as my sample showed), so I don't understand what you're saying there.

          Hmmm, I suppose this createSimilar jazz for arrays could be extended to consider element type for generics, but that goes beyond this issue (and there is Collection.flatten(Collection) which could be used).

          Show
          James P. White added a comment - - edited I wouldn't expect any different btw ( [1, 2, 3] as Integer[]).flatten() and ( [1, 2, 3] as Object[]).flatten() , just as there is no difference btw ([ [1, 2, 3] as Integer[]]).flatten() and ([ [1, 2, 3] as Object[]]).flatten() . So yes, the result is a List, which is what createSimilarOrDefaultCollection creates. And flatten as it is currently implemented is flattening multidimensional arrays (as my sample showed), so I don't understand what you're saying there. Hmmm, I suppose this createSimilar jazz for arrays could be extended to consider element type for generics, but that goes beyond this issue (and there is Collection.flatten(Collection) which could be used).
          Hide
          Robert Fischer added a comment -

          I do agree that either arrays should be flattened or they shouldn't – pick one. The inconsistency is confusing. I vote for not flattening them, on the basis that they aren't collections. But I really don't care, since I always "as List" my arrays as soon as they get dumped into Groovy.

          Unless there's been something awesome that I've missed out on, generic code is type-erased in Java, which means that the run-time has no idea what the generic type originally was, and there's no value in trying to discern or enforce it in "createSimilarOrDefaultCollection".

          Show
          Robert Fischer added a comment - I do agree that either arrays should be flattened or they shouldn't – pick one. The inconsistency is confusing. I vote for not flattening them, on the basis that they aren't collections. But I really don't care, since I always "as List" my arrays as soon as they get dumped into Groovy. Unless there's been something awesome that I've missed out on, generic code is type-erased in Java, which means that the run-time has no idea what the generic type originally was, and there's no value in trying to discern or enforce it in "createSimilarOrDefaultCollection".
          Hide
          Paul King added a comment -

          I wouldn't expect any different btw ([1, 2, 3] as Integer[]).flatten() and ([1, 2, 3] as Object[]).flatten(), just as there is no difference btw ([[1, 2, 3] as Integer[]]).flatten() and ([[1, 2, 3] as Object[]]).flatten(). So yes, the result is a List, which is what createSimilarOrDefaultCollection creates.

          Your interpretation is exactly the one I would expect; but you like I know about createSimilarOrDefaultCollection. I am just playing devil's advocate and wondering what our users think. Perhaps we just haven't defined createSimilarOrDefaultCollection fully yet to take into account arrays. If we say flatten is type preserving, why wouldn't I expect it to return an array? I think we can argue either way and I think we will end up with your interpretation, just not wanting to jump the gun.

          And flatten as it is currently implemented is flattening multidimensional arrays (as my sample showed), so I don't understand what you're saying there.

          Yes, but I can also imagine it would be useful to have for example a flatten which took int[][][] and returned int[].

          Show
          Paul King added a comment - I wouldn't expect any different btw ( [1, 2, 3] as Integer[]).flatten() and ( [1, 2, 3] as Object[]).flatten(), just as there is no difference btw ([ [1, 2, 3] as Integer[]]).flatten() and ([ [1, 2, 3] as Object[]]).flatten(). So yes, the result is a List, which is what createSimilarOrDefaultCollection creates. Your interpretation is exactly the one I would expect; but you like I know about createSimilarOrDefaultCollection . I am just playing devil's advocate and wondering what our users think. Perhaps we just haven't defined createSimilarOrDefaultCollection fully yet to take into account arrays. If we say flatten is type preserving, why wouldn't I expect it to return an array? I think we can argue either way and I think we will end up with your interpretation, just not wanting to jump the gun. And flatten as it is currently implemented is flattening multidimensional arrays (as my sample showed), so I don't understand what you're saying there. Yes, but I can also imagine it would be useful to have for example a flatten which took int[][][] and returned int[] .
          Hide
          James P. White added a comment -

          I'm not talking about Groovy's whole collection type system wrt arrays. I'm saying the current implementation of flatten is bugged and it needs some sort of fix.

          That said, I'm fairly sure that the only sensible interpretation is that Groovy coerces arrays to collections. To suppose that flatten might return an array rather than a collection also means that you would have functions like collect, find, and grep return an array rather than a collection when performed on an array. That wouldn't be a very good approach because any actual implementation would have to use variable sized collections and then convert to an array at the end, which is exactly the thing that the user will do if that is what he wants.

          Show
          James P. White added a comment - I'm not talking about Groovy's whole collection type system wrt arrays. I'm saying the current implementation of flatten is bugged and it needs some sort of fix. That said, I'm fairly sure that the only sensible interpretation is that Groovy coerces arrays to collections. To suppose that flatten might return an array rather than a collection also means that you would have functions like collect, find, and grep return an array rather than a collection when performed on an array. That wouldn't be a very good approach because any actual implementation would have to use variable sized collections and then convert to an array at the end, which is exactly the thing that the user will do if that is what he wants.
          Hide
          Paul King added a comment -

          @Jim, yes very thoughtful comments. I think you have all the thinking spot on. Apologies for erring on the side of socializing such thinking behind the language in this forum. Until we have that Wings language spec in place it can't hurt to discuss things a bit ... well, not hurt too much!?

          To reiterate your last comment and reinforce the need for further work on this issue, if findAll already converts arrays to lists as shown below, why not flatten:

          def nums = 1..10 as int[]
          assert nums instanceof int[]
          def evens = nums.findAll{ it % 2 == 0 }
          assert evens == [2, 4, 6, 8, 10]
          assert evens instanceof List
          

          I think that (along with earlier examples) seems pretty convincing to me.

          Show
          Paul King added a comment - @Jim, yes very thoughtful comments. I think you have all the thinking spot on. Apologies for erring on the side of socializing such thinking behind the language in this forum. Until we have that Wings language spec in place it can't hurt to discuss things a bit ... well, not hurt too much!? To reiterate your last comment and reinforce the need for further work on this issue, if findAll already converts arrays to lists as shown below, why not flatten : def nums = 1..10 as int [] assert nums instanceof int [] def evens = nums.findAll{ it % 2 == 0 } assert evens == [2, 4, 6, 8, 10] assert evens instanceof List I think that (along with earlier examples) seems pretty convincing to me.
          Hide
          Robert Fischer added a comment -

          If arrays are going to be lists, they should implement the full list API. Having arrays be half-lists from the view of duck typing just leads to confusion, bugs, and cognitive dissonance.

          Show
          Robert Fischer added a comment - If arrays are going to be lists, they should implement the full list API. Having arrays be half-lists from the view of duck typing just leads to confusion, bugs, and cognitive dissonance.
          Hide
          James P. White added a comment -

          No worries, Paul. Some of the RC issues and a lack of sleep and other such stuff have made be a bit cranky.

          Glad to see that we have arrived at consensus.

          Show
          James P. White added a comment - No worries, Paul. Some of the RC issues and a lack of sleep and other such stuff have made be a bit cranky. Glad to see that we have arrived at consensus.
          Hide
          Paul King added a comment -

          Amended to also handle arrays as base collection type

          Show
          Paul King added a comment - Amended to also handle arrays as base collection type

            People

            • Assignee:
              Paul King
              Reporter:
              Robert Fischer
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development