fix(core): validate Plan.Root output name count#812
Conversation
|
|
||
| @Value.Immutable | ||
| public abstract static class Root { | ||
| private static final Logger LOGGER = LoggerFactory.getLogger(Root.class); |
There was a problem hiding this comment.
I don't think this is a great solution. But considering that this update will result in stricter plan enforcement, I want to figure out a way where we can just warn on incorrect behavior for now, and then make it an actual error in the future.
But I'm not sure introducing a logger here where one isn't used anywhere else makes sense.
There was a problem hiding this comment.
I see. Left a comment on whether we want to make the spec more clear first. If the spec clearly says names are required then we can fire the exception straight away and we handle the change in behavior as a breaking change commit so it gets announced to consumers of substrait-java. That would then be sufficient in my opinion. Logging a warning to communicate the upcoming change is a nice gesture but who knows if consumers notice the warning message.
| if (actualNameCount == 0) { | ||
| LOGGER.warn( | ||
| "Plan.Root built without output names; this will be an error in the next release"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I have the feeling like we need to make the spec more clear on names being required for Plan.Root.
We probably also should flesh out the Plan Root documentation a little bit more to describe the structure and reference to the NamedStruct documentation.
What do you think?
| return ImmutableVirtualTableScan.builder(); | ||
| } | ||
|
|
||
| private static class NamedFieldCountingTypeVisitor |
There was a problem hiding this comment.
nice, I had no idea we already had such a visitor in the code base
2 participants
Adds a
Plan.Rootbuilder invariant for root output names.Empty
Plan.Root.namesare still allowed for now, but emit a warning saying this will become an error in the next release. When names are present, their count is validated against the input relation's depth-first named-field count, matching the Substrait named-struct rules.This also extracts named-field counting into a shared
NamedFieldCountingTypeVisitor, adds dedicated tests for the documented named-struct examples, and updates the plan roundtrip fixtures to match the repo's currentProjectoutput semantics.