Repository: frictionlessdata/datapackage-py (python)

Issue:
Improve header row handling in multipart tabular resource chunks
# 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.

# 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 ?

@pwalsh and @akariv your points of view and advices are more than welcome.

---

Please preserve this line to notify @roll (lead of this repository)


Pull request description:
headers management for multiparts in tabular resource
- fixes #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 https://github.com/frictionlessdata/datapackage-py/issues/256
---

Please preserve this line to notify @roll (lead of this repository)


Patch:
diff --git a/data/chunk2-with-headers.csv b/data/chunk2-with-headers.csv
new file mode 100644
index 0000000..a777bcc
--- /dev/null
+++ b/data/chunk2-with-headers.csv
@@ -0,0 +1,2 @@
+id,name
+2,中国人
diff --git a/datapackage/resource.py b/datapackage/resource.py
index aafab1d..cd50505 100644
--- a/datapackage/resource.py
+++ b/datapackage/resource.py
@@ -457,7 +457,7 @@ class Resource(object):
 
         # Get filelike
         if self.multipart:
-            filelike = _MultipartSource(self.source, remote=self.remote)
+            filelike = _MultipartSource(self)
         elif self.remote:
             if self.__table_options.get('http_session'):
                 http_session = self.__table_options['http_session']
@@ -657,7 +657,7 @@ class Resource(object):
             # Get source/schema
             source = self.source
             if self.multipart:
-                source = _MultipartSource(self.source, remote=self.remote)
+                source = _MultipartSource(self)
             schema = self.__current_descriptor.get('schema')
 
             # Storage resource
@@ -851,9 +851,17 @@ class _MultipartSource(object):
 
     # Public
 
-    def __init__(self, source, remote=False):
-        self.__source = source
-        self.__remote = remote
+    def __init__(self, resource):
+        # testing if we have headers
+        if resource.tabular \
+           and (resource.descriptor.get('dialect') and resource.descriptor.get('dialect').get('header')
+               or (not resource.descriptor.get('dialect') and config.DEFAULT_DIALECT['header'])):
+            remove_chunk_header_row = True
+        else:
+            remove_chunk_header_row = False
+        self.__source = resource.source
+        self.__remote = resource.remote
+        self.__remove_chunk_header_row = remove_chunk_header_row
         self.__rows = self.__iter_rows()
 
     def __enter__(self):
@@ -910,8 +918,30 @@ class _MultipartSource(object):
             streams = (urlopen(chunk) for chunk in self.__source)
         else:
             streams = (io.open(chunk, 'rb') for chunk in self.__source)
-        for stream in streams:
+        firstStream = True
+        header_row = None
+        for stream, chunk in zip(streams, self.__source):
+            firstRow = True
             for row in stream:
                 if not row.endswith(b'\n'):
                     row += b'\n'
-                yield row
+                # if tabular, skip header row in the concatenation stream
+                if firstRow and self.__remove_chunk_header_row:
+                    if firstStream:
+                        # store the first stream header row and yield it
+                        header_row = row
+                        yield row
+                    elif row == header_row:
+                        # remove header row of new stream is same as header from first stream
+                        pass
+                    else:
+                        # yield this first row but warn the user for deprecated situation
+                        # TODO: this warning might be removed in future releases ?
+                        warnings.warn("""%s has no headers whereas header = True.
+                            Deprecated legacy multi-part mode for tabular data.
+                            Headers will be required in chunks/multiparts in future.""" % chunk, UserWarning)
+                        yield row
+                else:
+                    yield row
+                firstRow = False
+            firstStream = False


Test patch:
diff --git a/tests/test_resource.py b/tests/test_resource.py
index 01ef44d..23a993c 100644
--- a/tests/test_resource.py
+++ b/tests/test_resource.py
@@ -481,7 +481,7 @@ def test_descriptor_table_tabular_multipart_local():
     descriptor = {
         'name': 'name',
         'profile': 'tabular-data-resource',
-        'path': ['chunk1.csv', 'chunk2.csv'],
+        'path': ['chunk1.csv', 'chunk2-with-headers.csv'],
         'schema': 'resource_schema.json',
     }
     resource = Resource(descriptor, base_path='data')
@@ -490,6 +490,36 @@ def test_descriptor_table_tabular_multipart_local():
         {'id': 2, 'name': '中国人'},
     ]
 
+def test_descriptor_table_tabular_multipart_noheader_local():
+    descriptor = {
+        'name': 'name',
+        'profile': 'tabular-data-resource',
+        'path': ['chunk2.csv', 'chunk2.csv'],
+        'schema': 'resource_schema.json',
+        'dialect': {'header': False},
+    }
+    resource = Resource(descriptor, base_path='data')
+    assert resource.table.read(keyed=True) == [
+        {'id': 2, 'name': '中国人'},
+        {'id': 2, 'name': '中国人'},
+    ]
+
+# test warning on legacy multipart header
+# TODO: to remove in future release ?
+def test_descriptor_table_tabular_multipart_mix_header_local():
+    descriptor = {
+        'name': 'name',
+        'profile': 'tabular-data-resource',
+        'path': ['chunk1.csv', 'chunk2.csv'],
+        'schema': 'resource_schema.json',
+        'dialect': {'header': True},
+    }
+    with pytest.warns(UserWarning):
+        resource = Resource(descriptor, base_path='data')
+        assert resource.table.read(keyed=True) == [
+            {'id': 1, 'name': 'english'},
+            {'id': 2, 'name': '中国人'},
+        ]
 
 def test_descriptor_table_tabular_multipart_remote(patch_get):
     descriptor = {
@@ -503,14 +533,15 @@ def test_descriptor_table_tabular_multipart_remote(patch_get):
         'schema': 'resource_schema.json',
     }
     # Mocks
-    patch_get('http://example.com/chunk1.csv', body="id,name\n")
-    patch_get('http://example.com/chunk2.csv', body="1,english")
-    patch_get('http://example.com/chunk3.csv', body="2,中国人\n")
+    patch_get('http://example.com/chunk1.csv', body="id,name\n1,english")
+    patch_get('http://example.com/chunk2.csv', body="id,name\n2,中国人\n")
+    patch_get('http://example.com/chunk3.csv', body="id,name\n3,français\n")
     # Tests
     resource = Resource(descriptor, base_path='data')
     assert resource.table.read(keyed=True) == [
         {'id': 1, 'name': 'english'},
         {'id': 2, 'name': '中国人'},
+        {'id': 3, 'name': 'français'},
     ]