Skip to content
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

import_matrix_table can import matrix table with no cols #5723

merged 4 commits into from Apr 2, 2019


Copy link

@catoverdrive catoverdrive commented Mar 28, 2019

Fixes #5718.

@@ -37,6 +35,8 @@ class LoadMatrixParser(rvb: RegionValueBuilder, fieldTypes: Array[Type], entryTy
| Line:
| ${ line.truncate }""".stripMargin
off = addType(fieldTypes(ii))(line, rowNum, ii, off)
Copy link

@jigold jigold Mar 28, 2019

Your test looks good. How does this change accomplish what you wanted? It looks like a simple reorder to me.

Copy link
Contributor Author

@catoverdrive catoverdrive Mar 28, 2019

The old way, we were checking that the line hadn't ended after every row field was added, which means that for something with no columns, we were checking that the line hadn't ended... at the end of the line.

This way, we're doing the check before adding each row field, so we'll only throw an error if we expect to be able to add another field and we can't. I think that's the only change necessary for this to work, but I wasn't able to get my dev env set up fully this week so I hadn't tested it locally.

jigold approved these changes Mar 28, 2019
Copy link

@jigold jigold commented Apr 1, 2019

@catoverdrive This looks like it's been running for 2 hours now. I would have expected it to be merged already as well. Could this cause an infinite loop -- you didn't test it locally correct?

Copy link
Contributor Author

@catoverdrive catoverdrive commented Apr 1, 2019

Just fixed it

@danking danking merged commit 4a752ab into hail-is:master Apr 2, 2019
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants