ARROW-8290: [Python] Improve FileSystemDataset constructor #6913
Conversation
| FileFormat file_format not None, | ||
| FileSystem filesystem not None, | ||
| paths_or_selector, partitions): | ||
| def __init__(self, paths_or_selector, schema=None, format=None, |
There was a problem hiding this comment.
@jorisvandenbossche I think you can leave the type annotations here, just the not None part needs to be removed.
There was a problem hiding this comment.
The problem is then that it are positional keywords to cython. And although you can call it as keyword arg, you get all kinds of uninformative error messages once you do something wrong.
(maybe there is a better way to do this with cython, but didn't directly figure out. There is also not much docs about this)
There was a problem hiding this comment.
Like "TypeError: init() takes exactly 6 positional arguments (1 given)" when using all keyword arguments, but having a typo in one of them.
This was actually the reason that I first thought that you needed to pass everything as positional arguments, because I used format= instead of file_format=, and then you get such error. And so I wanted to solve that in this PR to allow keyword arguments, to find out that the cause was actually a wrong name and that keyword arguments were already supported .. :)
There was a problem hiding this comment.
Indeed, that's annoying. Do you want to report this upstream?
There was a problem hiding this comment.
Yes, I can do that. Need to make a small reproducer then, will put that on my to do list.
There was a problem hiding this comment.
@jorisvandenbossche schema, format and filesystem arguments are mandatory, please remove the default values.
There was a problem hiding this comment.
I know it might be confusing from the signature, but I did this to ensure proper error messages (and the docstring indicates which are optional, and you will get a proper error message when not passing a value)
| root_partition.unwrap(), | ||
| file_format.unwrap(), | ||
| filesystem.unwrap(), | ||
| (<Expression> root_partition).unwrap(), |
There was a problem hiding this comment.
If I pass an object which is not castable to <Expression> then I get a segfault, e.g. root_partition=1
There was a problem hiding this comment.
You change the cast to <Expression?> then it is raising a cannot convert type error - although it might be better to set default values before the casts.
There was a problem hiding this comment.
Ah, yes, all other keywords are validated to be of the correct class before casting, except this one, thanks for the catch!
There was a problem hiding this comment.
OK, fixed this
schema,formatandfilesystemkeyword validation manually, instead of letting cython handle it (which gives very cryptic error messages, like complaining about positional arguments if you made a typo in one of the keyword names)root_expressionto the endpartitionsandroot_expressionoptionalfile_formattoformat, since this name is what we are using elsewhere (eg inds.dataset(..)