Store array

[ostephens]

Demonstration for #1088
Looking for comments on this approach rather than approval at this stage

[thadguidry]

I tested the functionality a bit... with the following value.smartSplit("|||") just to be devious 😁

record|||Alfredo Santamaria 2013|||Spain|||Castilla-y-León|||Cigales	|||Alfredo Santamaria Arias, S.L. (Producer)|||+34 983 58 50 06|||www.bodega-santamaria.com

and got this result

[ "record", "", "", "Alfredo Santamaria 2013", "", "", "Spain", "", "", "Castilla-y-León", "", "", "Cigales\t", "", "", "Alfredo Santamaria Arias, S.L. (Producer)", "", "", "+34 983 58 50 06", "", "", "www.bodega-santamaria.com" ]

Notice, I don't have 3 elements between "record" and "Alfredo Santamaria 2013", etc. but instead only 2.

Did I lose the 3rd pipe ? or 1st pipe ? Why did I lose it ? Was this because of smartSplit() function itself ?

You probably know this, since you mentioned "limited tests" but ensure we use both partition() and rpartition() as part of our tests, and once we figure our smartSplit() reasons above, add that one also.

And great job putting this together so far ! You da man !

[jackyq2015]

Left the inline comments

[jackyq2015]

Not sure the exact reason why the array of list were not designed as storable originally. But one of the reason might be storable also means recoverable. A array/list of string generated by split function is recoverable. But a list of WrappedCell or WrappedRow are not recoverable. So might be good idea to narrow the scope of the isArrayOrList

[ostephens]

Agreed that we don't want to store a WrappedCell or WrappedRow. I believe isArrayOrList returns false for WrappedCell or WrappedRow. isArrayOrList checks for v.getClass().isArray() || v instanceof List<?>

[jackyq2015]

I mean array of wrapped cell , row.

[ostephens]

Ah - thanks @jackyq2015 I'll have a look

[ostephens]

@thadguidry thanks for testing. I think my expectation is that using smartSplit on:
record|||Alfredo Santamaria 2013

should return:

[ "record", "", "", "Alfredo Santamaria 2013" ]

The three pipes between 'record' and 'Alfredo' translate to two fields - so e.g.

record| 1 | 2 |Alfredo Santamaria 2013 -> [ "record", "1", "2", "Alfredo Santamaria 2013" ]

Is your expectation different?

[thadguidry]

@ostephens Its a smartSplit() issue. Looks like David Huynh never went in and added the multichar support (I checked the code and history in link below) and so it is only looking at a single char for the sep String instead of any number of characters for sep. This was added as an enhancement for split() and also on the menu for "Split into several columns..." and the reason I know this is because I asked @dfhuynh to change that functionality back in 2010, which he did. BUT...doesn't look like we did that for smartSplit() .... so yeah... smartSplit() only takes the 1st character in the sep argument supplied and ignores any other characters in the argument.

Notice here it splits on everything with "r" ... but not my supplied "rd"
I'd say that's a enhancement from 2010 that I asked for that never happened in smartSplit() 😀 and not a classic bug 😄

So your Store Array functionality is fine. And works great from my testing on all sides, so far.

Issue is our inaccurate documentation and description of smartSplit() saying it works with a sep String, when it fact it only works with a sep Char ... and make a change to have it work like the other split functions and allow sep to be a String and not just charAt(0) , and keeping its "smarts" with special handling of quotes and guessing tabs or commas of sep String is not supplied.

@ostephens @ettorerizza @wetneb If you agree on above enhancement with smartSplit() then create new issue with above info copied in.

[wetneb]

Looks good to me. It makes sense to unify preview and render code.

change of mind - I am not sure it is safe to introduce a new possible java type for cell values.

[wetneb]

Explanation of the above:

I think it is a design mistake to have allowed arbitrary java Objects as cell values. It would have been much safer to restrict it to a specified list of java types, so that the compiler would enforce correct typing of all values. I believe this choice was made to make it easy for extensions to manipulate complex objects - but this extendability does not work well in many parts of the code: there is no simple way for an extension to implement the expected behaviour of these objects in rendering, faceting, and other operations. So type safety should really prevail here. We have already paid the price of this design flaw when migrating the dates to java 8 classes.

So, adding a new possible type is dangerous - the compiler cannot spot for us the other places in our own code that we would need to adapt for array handling. And the compiler will not let extension maintainers that they might need to update their own code too.

[jackyq2015]

Object might be too much. A well defined interface may able to repalce it

[ostephens]

This PR is pretty stale by now. I'm not really sure how we should progress this. I'm going to close this right now - if we want to pick this up we probably need some more thought about exactly what we want from that behaviour