Logic bug in arrow_writer?
{'login': 'cccntu', 'id': 31893406, 'node_id': 'MDQ6VXNlcjMxODkzNDA2', 'avatar_url': 'https://avatars.githubusercontent.com/u/31893406?v=4', 'gravatar_id': '', 'url': 'https://api.github.com/users/cccntu', 'html_url': 'https://github.com/cccntu', 'followers_url': 'https://api.github.com/users/cccntu/followers', 'following_url': 'https://api.github.com/users/cccntu/following{/other_user}', 'gists_url': 'https://api.github.com/users/cccntu/gists{/gist_id}', 'starred_url': 'https://api.github.com/users/cccntu/starred{/owner}{/repo}', 'subscriptions_url': 'https://api.github.com/users/cccntu/subscriptions', 'organizations_url': 'https://api.github.com/users/cccntu/orgs', 'repos_url': 'https://api.github.com/users/cccntu/repos', 'events_url': 'https://api.github.com/users/cccntu/events{/privacy}', 'received_events_url': 'https://api.github.com/users/cccntu/received_events', 'type': 'User', 'site_admin': False}
[]
[]
["Hi @cccntu you're right, as when `batch_examples={}` the current if-statement won't be triggered as the condition won't be satisfied, I'll prepare a PR to address it as well as add the regression tests so that this issue is handled properly."
 'Hi @alvarobartt ,\r\nThanks for answering. Do you know when and why an empty batch is passed to this function? This only happened to me when processing with multiple workers, while chunking examples, I think.'
 "> Hi @alvarobartt , Thanks for answering. Do you know when and why an empty batch is passed to this function? This only happened to me when processing with multiple workers, while chunking examples, I think.\r\n\r\nSo it depends on how you're actually chunking the data as if you're not handling empty chunks `batch_examples={}` or `batch_examples=None`, you may end up running into this issue. So you could check the chunks before you actually call `ArrowWriter.write_batch`, but anyway the fix you proposed I think improves the logic of `write_batch` to avoid running into these issues."
 'Thanks, I added a if-print and I found it does return an empty examples in the chunking function that is passed to `.map()`.'
 'Hi ! We consider an empty batch to look like this:\r\n```python\r\nempty_batch = {\r\n    "column_1": [],\r\n    "column_2": [],\r\n    ...\r\n}\r\n```\r\n\r\nWhile `{}` corresponds to a batch with no columns.\r\n\r\nTherefore calling this code should fail, because the two batches don\'t have the same columns:\r\n```python\r\nwriter.write_batch({"a": [1, 2, 3]})\r\nwriter.write_batch({})\r\n```\r\n\r\nIf you want to write an empty batch, you should do this instead:\r\n```python\r\nwriter.write_batch({"a": [1, 2, 3]})\r\nwriter.write_batch({"a": []})\r\n```'
 'Makes sense, then the if-statement should remain the same or is it better to handle both cases separately using `if not batch_examples or len(next(iter(batch_examples.values()))) == 0: ...`?\r\n\r\nUpdating the regressions tests with an empty batch formatted as `{"col_1": [], "col_2": []}` instead of `{}` works fine with the current if, and also with the one proposed by @cccntu.'
 '> Makes sense, then the if-statement should remain the same or is it better to handle both cases separately using if not batch_examples or len(next(iter(batch_examples.values()))) == 0: ...?\r\n\r\nThere\'s a check later in the code that makes sure that the columns are the right ones, so I don\'t think we need to check for `{}` here\r\n\r\nIn particular the check `if not batch_examples or len(next(iter(batch_examples.values()))) == 0:` doesn\'t raise an error while it should, that why the old `if` is fine IMO\r\n\r\n> Updating the regressions tests with an empty batch formatted as {"col_1": [], "col_2": []} instead of {} works fine with the current if, and also with the one proposed by @cccntu.\r\n\r\nCool ! If you want you can update your PR to add the regression tests, to make sure that `{"col_1": [], "col_2": []}` works but not `{}`'
 "Great thanks for the response! So I'll just add that regression test and remove the current if-statement."
 'Hi @lhoestq ,\r\n\r\nThanks for your explanation. Now I get it that `{}` means the columns are different. But wouldn\'t it be nice if the code can ignore it, like it ignores `{"a": []}`?\r\n\r\n\r\n--- \r\nBTW, \r\n> There\'s a check later in the code that makes sure that the columns are the right ones, so I don\'t think we need to check for {} here\r\n\r\nI remember the error happens around here:\r\nhttps://github.com/huggingface/datasets/blob/88a902d6474fae8d793542d57a4f3b0d187f3c5b/src/datasets/arrow_writer.py#L506-L507\r\nThe error says something like `arrays` and `schema` doesn\'t have the same length. And it\'s not very clear I passed a `{}`.\r\n\r\nedit: actual error message\r\n```\r\nFile "site-packages/datasets/arrow_writer.py", line 595, in write_batch\r\n    pa_table = pa.Table.from_arrays(arrays, schema=schema)\r\n  File "pyarrow/table.pxi", line 3557, in pyarrow.lib.Table.from_arrays\r\n  File "pyarrow/table.pxi", line 1401, in pyarrow.lib._sanitize_arrays\r\nValueError: Schema and number of arrays unequal\r\n```'
 '> But wouldn\'t it be nice if the code can ignore it, like it ignores {"a": []}?\r\n\r\nI think it would make things confusing because it doesn\'t follow our definition of a batch: "the columns of a batch = the keys of the dict". It would probably break certain behaviors as well. For example if you remove all the columns of a dataset (using `.remove_colums(...)` or `.map(..., remove_columns=...)`), the writer has to write 0 columns, and currently the only way to tell the writer to do so using `write_batch` is to pass `{}`.\r\n\r\n> The error says something like arrays and schema doesn\'t have the same length. And it\'s not very clear I passed a {}.\r\n\r\nYea the message can actually be improved indeed, it\'s definitely not clear. Maybe we can add a line right before the call `pa.Table.from_arrays` to make sure the keys of the batch match the field names of the schema']
https://github.com/huggingface/datasets/blob/88a902d6474fae8d793542d57a4f3b0d187f3c5b/src/datasets/arrow_writer.py#L475-L488

I got some error, and I found it's caused by `batch_examples` being `{}`. I wonder if the code should be as follows:
```
-        if batch_examples and len(next(iter(batch_examples.values()))) == 0:
+       if not batch_examples or len(next(iter(batch_examples.values()))) == 0:
            return
```
@lhoestq 
{'url': 'https://api.github.com/repos/huggingface/datasets/issues/4502/reactions', 'total_count': 0, '+1': 0, '-1': 0, 'laugh': 0, 'hooray': 0, 'confused': 0, 'heart': 0, 'rocket': 0, 'eyes': 0}