Skip to content

Transpose cells across columns into Rows BUGS with blank cells? #5229

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

Closed
fabiolinus opened this issue Sep 2, 2022 · 11 comments · Fixed by #5862
Closed

Transpose cells across columns into Rows BUGS with blank cells? #5229

fabiolinus opened this issue Sep 2, 2022 · 11 comments · Fixed by #5862
Assignees
Labels
Good First Issue Indicates issues suitable for newcomers to design or coding, providing a gentle introduction. transpose/pivot Improving / adding transpose, pivot operations Type: Bug Issues related to software defects or unexpected behavior, which require resolution.
Milestone

Comments

@fabiolinus
Copy link

Hi. Today I used a menu called 'Transpose cells across columns into Rows'. I checked on 'Ignore blank cells' but it failed. I think that the mistake is on the label. It works with null cells but not with empty cells. So It can't work with blank cells whis is empty OR null. Can you verify it? Thanks a lot, Fabio
Untitled

@fabiolinus fabiolinus added Type: Bug Issues related to software defects or unexpected behavior, which require resolution. Status: Pending Review Indicates that the issue or pull request is awaiting review by project maintainers or collaborators labels Sep 2, 2022
@wetneb wetneb added the transpose/pivot Improving / adding transpose, pivot operations label Sep 2, 2022
@thadguidry thadguidry removed the Status: Pending Review Indicates that the issue or pull request is awaiting review by project maintainers or collaborators label Oct 7, 2022
@thadguidry
Copy link
Member

thadguidry commented Oct 7, 2022

@fabiolinus it would have never worked with blank (empty cells) (prior to 3.0+ null cell changes and prior back before 2.7) because we made no changes to that part of the code and it is only checking cell == null || cell.value == null as seen in the git blame: https://github.com/OpenRefine/OpenRefine/blame/master/main/src/com/google/refine/operations/cell/TransposeColumnsIntoRowsOperation.java#L307

@ostephens what do you think of keeping the handling as-is and updating the label to coincide with the current code of only null handling, or do we indeed make it slightly more useful to include both empty string and null cell handling with 2 check boxes? We probably need some tests added for transpose. CORRECTION: We definitely need tests added - there are NONE !

@ostephens
Copy link
Member

@ostephens what do you think of keeping the handling as-is and updating the label to coincide with the current code of only null handling, or do we indeed make it slightly more useful to include both blank and null cell handling with 2 check boxes?

We should definitely make sure the terminology here aligns with our use elsewhere - we should always use 'blank' to mean "either empty string (string with length zero) or null" across OpenRefine for consistency.

In terms of the behaviour here I don't have strong opinion. The usual approach we take is to give the user the choice of:

  • Null
  • Empty string
  • Blank (null or empty string)

However - I suspect for the majority of users this distinction isn't one that is meaningful (hence having the 'blank' concept in the first place!).

We've had other transpose behaviour issues arising before (e.g. #3781) and the behaviour seems difficult to understand. Given your comment about tests I wonder if this is an area which needs some investment - both in terms of understanding what users actually need in terms of transpose functionality, and making sure the code and options support it!

So two suggestions:

  1. Update the label here ASAP - this is an easy fix, and at least makes clear what's going on
  2. See if someone would be willing to do some further investigatory work on what users want from transpose (with the ultimate aim of writing some tests and bringing our code in line with the expected behaviours)

Obviously if 2 isn't possible in a reasonable timescale we should get someone to write test cases for the current behaviour to prevent regressions!

@thadguidry
Copy link
Member

I do have an old project attached to issue #1230 that may/may not help with a test for regression (circa 2017 with OpenRefine 2.7)

@wetneb wetneb added the Good First Issue Indicates issues suitable for newcomers to design or coding, providing a gentle introduction. label Feb 7, 2023
@ayushrai206
Copy link
Member

Hey,i am Ayushi an outreachy 2023 applicant, can i have this issue assigned to me?

@fabiolinus
Copy link
Author

fabiolinus commented Mar 14, 2023

Hi ayushrai. Please, see the attached csv file. Try to load it on openrefine in both ways. In other words select and deselect an option called 'Store blank cells as nulls':

image

In the first case you have a null cell (missing value), in the other case you have an empty cell (''). Then use 'Transpose cells across columns into Rows' with checked 'Ignore blank cells'. You can see two different behaviours. I think this is not correct because blank is null OR empty so It should work anyway. I hope it is clear. Thanks a lot, Fabio

wide_ayushrai206.csv

@ayushrai206 ayushrai206 removed their assignment Mar 14, 2023
@jenny-Musah
Copy link
Contributor

jenny-Musah commented Mar 26, 2023

@fabiolinus , @thadguidry I would like to attempt to fix this, but I don't think we've concluded on what change we would like to make, and I'm a bit scared as there is no test. What is the conclusion?

@fabiolinus
Copy link
Author

Hi. I am not a programmer. I have no news but I wrote some details about it in my last comment . How can I help you?

@skhoylow8
Copy link
Contributor

Hi. I saw this issue that thought it would be a good issue to work on. May I be assigned to it? Also just to clarify, do we want to change the label from "Ignore blank cells" to "Ignore null cells" or should we change the if statement to include blank/empty strings? Thanks in advance for your help.

@wetneb
Copy link
Member

wetneb commented May 18, 2023

I think both approaches would be appropriate, so it's up to you!

@skhoylow8
Copy link
Contributor

Hi. I was wondering if someone can give me some advice on how to write some test cases for the transpose columns into rows functionality. I would like to write test cases to make sure the correct behavior is being executed with both a null and empty cell. Thanks.

@wetneb
Copy link
Member

wetneb commented May 23, 2023

The approach you have taken in #5862 looks very good to me.

wetneb pushed a commit that referenced this issue Jun 2, 2023
* Fixed the issue with transpose where it didnt treat blank cells as null

* Ran command to format java file

* Cleaned up null and empty string checking for transpose

Closes #5229
@wetneb wetneb added this to the 3.8 milestone Jun 2, 2023
wetneb pushed a commit that referenced this issue Jun 26, 2023
* Fixed the issue with transpose where it didnt treat blank cells as null

* Ran command to format java file

* Cleaned up null and empty string checking for transpose

Closes #5229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Indicates issues suitable for newcomers to design or coding, providing a gentle introduction. transpose/pivot Improving / adding transpose, pivot operations Type: Bug Issues related to software defects or unexpected behavior, which require resolution.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants