headers management for multiparts in tabular resource

[paulgirard]

• fixes Improve header row handling in multipart tabular resource chunks #256

---

Overview

A multipart resource concatenates chunks as-is which is what we want for generic binary files.
But for tabular resource this default behaviour implies that if the first chunk does have one header row, the other chunks must not.
We discussed this issue here: https://github.com/frictionlessdata/forum/issues/1
The proposition is to change this behaviour for tabular-data-package: tabular chunks should all have headers or none depending on dialect.header.

Current situation with header row only in first chunck are handled as before but it raises a UserWarning as this situation will soon be deprecated.

Implementation

Multipart chunks are handled by the _MultipartSource class which build an iterator of chunks' rows iterator. My approach is simply to discard first row of chunks (but the first) iterator when the resource is tabular with header.
@roll pointed possible issues with:

• resource.raw_read: I've checked this method uses _MultipartSource iterator so should be safe
• datapackage.save: I am not sure to understand the risk in there since data are only read from resources. Saving only writes the datapackage not the data itself right ?

Previously posted at #256

Please preserve this line to notify @roll (lead of this repository)

[paulgirard]

Sorry my local tests were not working with python3...
I have to solve those issues.

[paulgirard]

That's better.
I changed the way to discard header row from multipart streams to make it compliant with 3.x buffer implementation.

@roll this PR is ready for your review

[roll]

Hi thanks! I'm going to start it testing across the stack this week

[paulgirard]

Awesome !

Actually I got an idea about this implementation.
This implementation requires tabular datapackage ressources' multiparts to all have the same header configuration. Either all have headers or none.
This is a breaking change compared to the previous implementation.
If you'd like to limit the pain for users to comply with this breaking change we could add some magic - I use magic as a signal of "maybe not such a good idea IMHO".
But still I let it out.

In case of a with-header tabular multipart resource, the implementation could test if the first line of multiparts are always the same as the one from the first part (at the byte level) before discarding it. If not we could raise a no-header error rather than silently discard data rows (which were supposed to be header ones).
This feature would avoid messing up loading process of "older-not-consistently-headered" tabular multipart resource.

What do you think ?

[roll]

@paulgirard
It seems the implementation is great 👍
I was afraid that it would be much harder.

I'm going to think on Monday how to release it properly regarding backward-compatibility...

[paulgirard]

\o/
Glad there were no hidden issues!
Very happy to help datapackage community (starting by my own projects :)

[roll]

Hi @paulgirard, here are my thoughts. Could you please:

• compare the first row of the rest chunks to the main one
• if it matches
  • drop it from the data stream (as you have already implemented)
• if it doesn't match
  • preserve it in the data stream
  • warnings.warn a UserWarning saying that it's a deprecated legacy multi-part mode for tabular data and headers will be required since the next major version of datapackage
  • probably add TODO saying that in the next major version we need to remove this warning and compare headers to raise not matching header error

What do you think? Does it make sense?

[paulgirard]

Yeah that's pretty close to what I had in mind in my last comment.
This will ease adoption of the breaking change.
I'll do that in a few days.

[roll]

Great! Thanks a lot!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[paulgirard]

@roll I finally had time to add the new behaviour for deprecated no-header in chunks situation as you asked.
Please consider reopening this PR.
Sorry for the delay.

[roll]

Hi @paulgirard, sure

[roll]

Thanks. It looks good

[paulgirard]

Nice ! Sorry this took so long and so many small commits for such a straight forward feature...
Glad to have contributed to a better datapackage-py ;)
If anyone needs a use case of this feature, see : http://github.com/medialab/ricardo_data or http://github.com/medialab/toflit18_data (datapackage-py not yet completely integrated)

[roll]

Great, thanks!