Logic bug in arrow_writer?
Opened by changjonathanc (closed)

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 

Comments:
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 ,
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.

> 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.

So 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:
```python
empty_batch = {
    "column_1": [],
    "column_2": [],
    ...
}
```

While `{}` corresponds to a batch with no columns.

Therefore calling this code should fail, because the two batches don't have the same columns:
```python
writer.write_batch({"a": [1, 2, 3]})
writer.write_batch({})
```

If you want to write an empty batch, you should do this instead:
```python
writer.write_batch({"a": [1, 2, 3]})
writer.write_batch({"a": []})
```

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: ...`?

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.

> 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: ...?

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

In 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

> 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.

Cool ! 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 ,

Thanks 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": []}`?


--- 
BTW, 
> 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

I remember the error happens around here:
https://github.com/huggingface/datasets/blob/88a902d6474fae8d793542d57a4f3b0d187f3c5b/src/datasets/arrow_writer.py#L506-L507
The error says something like `arrays` and `schema` doesn't have the same length. And it's not very clear I passed a `{}`.

edit: actual error message
```
File "site-packages/datasets/arrow_writer.py", line 595, in write_batch
    pa_table = pa.Table.from_arrays(arrays, schema=schema)
  File "pyarrow/table.pxi", line 3557, in pyarrow.lib.Table.from_arrays
  File "pyarrow/table.pxi", line 1401, in pyarrow.lib._sanitize_arrays
ValueError: Schema and number of arrays unequal
```

> But wouldn't it be nice if the code can ignore it, like it ignores {"a": []}?

I 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 `{}`.

> The error says something like arrays and schema doesn't have the same length. And it's not very clear I passed a {}.

Yea 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