feat: removing emptyScan in favor of emptyVirtualTableScan #682
Conversation
|
ACTION NEEDED Substrait follows the Conventional Commits The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
gord02
force-pushed
the
gordon.hamilton/removeEmptyScan
branch
from
a66fe19 to
0c267da
Compare
gord02
changed the title
benbellick
left a comment
benbellick
left a comment
There was a problem hiding this comment.
all looks reasonable to me! Only thing really is just putting that roundtrip test back in.
Also, maybe we will want to mark this as a breaking change since we are technically modifying the API here by dropping a public class. @vbarua
| public VirtualTableScan emptyVirtualTableScan() { | ||
| return VirtualTableScan.builder() | ||
| .initialSchema(NamedStruct.of(Collections.emptyList(), R.struct())) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
| public VirtualTableScan emptyVirtualTableScan() { | |
| return VirtualTableScan.builder() | |
| .initialSchema(NamedStruct.of(Collections.emptyList(), R.struct())) | |
| .build(); | |
| } |
I'm almost inclined to just delete this entirely now. Is this also schemaless?
There was a problem hiding this comment.
Yeah I think that if removing it isn't such a large diff, then I am in support of getting rid of it. If it ends up being a bit of a pain though, we can save it for later.
There was a problem hiding this comment.
Okay I see now its used in a bunch of tests, so no worries about keeping it for now.
| void emptyScan() { | ||
| io.substrait.relation.EmptyScan emptyScan = sb.emptyScan(); | ||
| verifyRoundTrip(emptyScan); | ||
| } |
There was a problem hiding this comment.
I think we might as well just keep this test but make it a roundtrip test on the empty virtual table.
2 participants
This PR removes the EmptyScan and replaces it with empty VirtualTableScan.
Closes #673