Conversation
|
cc @jacques-n |
jacques-n
left a comment
jacques-n
left a comment
There was a problem hiding this comment.
This generally looks good. I think it probably makes sense to make each of the formats empty classes instead of enums so that we can introduce options on each of those formats later without having breaking changes. Can you make that change?
Sure, let me first verify it in the gluten and see whther it's ok or not, it could take me a while. |
baibaichen
force-pushed
the
feature/LocalFiles
branch
from
952c3ea to
9d49e58
Compare
jacques-n
left a comment
jacques-n
left a comment
There was a problem hiding this comment.
Can you please add roundtrip tests to this?
|
|
||
| @Override | ||
| public RelNode visit(LocalFiles localFiles) throws RuntimeException { | ||
| throw new UnsupportedOperationException("LocalFiles is not supported"); |
There was a problem hiding this comment.
Why are you throwing here?
There was a problem hiding this comment.
Why are you throwing here?
using visitFallback instead of throwing an exception directly for unsupported rel
| import java.util.Arrays; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class LocalFilesRoundtripTest { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, apparently missed that on first pass. Thanks
* feat(relation): add read relation LocalFiles * fix: using visitFallback instead of throwing an exception directly
2 participants
Currently substrait-java doesn't implement
LocalFilesReadRel, this patch adds this missingRel.How to test
Add a new UT
LocalFilesRoundtripTest