Columnize by key/value columns creates empty lines

[hvmarck]

When using the following data set (in CSV format with ; separator):

ID;Cat;Val
1;a;1
1;b;3
2;b;4
2;c;5
3;a;2
3;b;5
3;d;3

and using the 'Columnize by key/value columns' with OpenRefine 2.6 beta 1 it creates extra blank lines (in CSV format again):

ID;a;b;c;d
;;;;
1;1;3;;
2;;4;5;
;;;;
3;2;5;;3

while it should have been (as done with version 2.5):

ID;a;b;c;d
1;1;3;;
2;;4;5;
3;2;5;;3

[tfmorris]

This regression was apparently introduced by ca2e959 when fixing issue #529

[magdmartin]

@jackyq2015 can you check #849 and comment if it fix the issue.

[thadguidry]

Jacky

Can you add those comments directly to #796 issue ? Thanks.

BTW, Markdown is supported on comments in Github if you want to prettify
that alignment - https://guides.github.com/features/mastering-markdown/

Thad
+ThadGuidry https://www.google.com/+ThadGuidry

On Tue, Sep 22, 2015 at 6:10 PM, QI notifications@github.com wrote:

This is the result I got after applied the patch made by @ultraklon
https://github.com/ultraklon
Id a b c d
1

2 1 1 3
3 2 4 5
4
5 3 2 5 3

But when I export the csv, It's fine(only 3 lines). Maybe it's because the
empty line were removed when exporting.

So It's not a completely fix me.

—
Reply to this email directly or view it on GitHub
#796 (comment)
.

[jackyq2015]

Tested again @ultraklon PR, It works. But as he pointed out, seems there's some discrepancy between the test result from the UI and from the Unit testing.

Will be looking into it.

[jackyq2015]

@ultraklon, for the PR #849 you created. To answer your concern for few null cell values, it's a side effect of the reuse row which aimed to address the performance. The post-operation column model is actually not consistent with the data itself because of the reuse. Ie, the cell[1], cell[2] as the old cell values should have been removed completely. Some cleanup has to be done for this operation on the column model and row model. Also releasing the null reference can free up some memory especially for big data set.

Also for the UT, the expected cell number will be differ from row to row since not all the 5 cells are fully populated. so the expectation should also get changed.

Will be working on the 2 changes mentioned above and hopefully can submit the PR this week.

[jackyq2015]

PR created. It was based on the change @ultraklon made.

[thadguidry]

@jackyq2015 still not fixed completely. See #1098

[jackyq2015]

dup of #1138