[CALCITE-7321] FilesTableFunction throws NumberFormatException on macOS with GNU stat installed#4672
Conversation
asolimando
force-pushed
the
main-CALCITE-7321_osadapter_filestablefunction
branch
from
b9cbde7 to
233b2cb
Compare
…OS with GNU stat installed
asolimando
force-pushed
the
main-CALCITE-7321_osadapter_filestablefunction
branch
from
233b2cb to
bcbed0c
Compare
|
| return Processes.processLines('\n', args); | ||
| } | ||
|
|
||
| private Enumerable<String> sourceGnuStat() { |
There was a problem hiding this comment.
I think it would be a bit nicer to encapsulate the stat abstraction in an interface with multiple implementations
There was a problem hiding this comment.
I think it’s fine to put it in the FilesTableFunction as well. It seems that sourceGnuStat won’t be used anywhere else.
There was a problem hiding this comment.
I agree with the design principle, but as @xiedeyantu noted, this is an implementation detail specific to this table function and unlikely to be reused elsewhere, so I'd be inclined to leave it as-is
There was a problem hiding this comment.
(if you don't feel strongly I'd rather land this as-is, I am not particularly in a rush but the whole PR is to regain the ability to execute tests locally, in my previous PR I missed some real test issues due to that as I never get a green run due to this class not working on my setup, which I don't manage)
There was a problem hiding this comment.
Since you approved already I will consider it good to go if there are no objections in around 24h, thanks!
plus/src/main/java/org/apache/calcite/adapter/os/FilesTableFunction.java
Show resolved
Hide resolved
plus/src/main/java/org/apache/calcite/adapter/os/FilesTableFunction.java
Show resolved
Hide resolved
asolimando
added
the
LGTM-will-merge-soon
asolimando
deleted the
main-CALCITE-7321_osadapter_filestablefunction
branch
|
Thanks @mihaibudiu, @xiedeyantu and @xuzifu666 for your reviews! |
4 participants
Details in CALCITE-7321