feat(core): add MaskExpression POJO and ReadRel projection support#782
feat(core): add MaskExpression POJO and ReadRel projection support#782nielspardon merged 17 commits intosubstrait-io:mainfrom
Conversation
|
Hi @flex-seongmin, thanks for your contribution. I enabled the Github Action workflow and it looks like your PR has some formatting issues. Running |
nielspardon
left a comment
There was a problem hiding this comment.
I have some initial structural suggestions for the code from my side
core/src/main/java/io/substrait/expression/proto/MaskExpressionProtoConverter.java
Show resolved
Hide resolved
flex-seongmin
left a comment
There was a problem hiding this comment.
Thanks for kind direction for sync code convention and architectural consistency.
I added 3 commits.
core/src/main/java/io/substrait/expression/proto/MaskExpressionProtoConverter.java
Show resolved
Hide resolved
core/src/main/java/io/substrait/expression/proto/MaskExpressionProtoConverter.java
Show resolved
Hide resolved
core/src/main/java/io/substrait/expression/proto/ProtoMaskExpressionConverter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/substrait/expression/proto/MaskExpressionProtoConverter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/substrait/expression/MaskExpressionTypeProjector.java
Outdated
Show resolved
Hide resolved
flex-seongmin
left a comment
There was a problem hiding this comment.
I committed that you reviewed! Thanks for review.
- Added javadoc for class / public functions
- Use
MaskExpressionVisitorfor Select dispatc.
core/src/main/java/io/substrait/expression/MaskExpressionTypeProjector.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/substrait/expression/MaskExpressionTypeProjector.java
Outdated
Show resolved
Hide resolved
|
if you can fix the javadoc and the minor renaming I think this is looking good |
core/src/main/java/io/substrait/expression/proto/ProtoMaskExpressionConverter.java
Outdated
Show resolved
Hide resolved
…ble with @Value.Enclosing
| @Value.Immutable | ||
| public interface MaskExpression { | ||
|
|
||
| /** The top-level struct selection describing which fields to include. */ |
There was a problem hiding this comment.
I think this is also missing a @return
There was a problem hiding this comment.
| /** The top-level struct selection describing which fields to include. */ | |
| /** | |
| * The top-level struct selection describing which fields to include. | |
| * | |
| * @return the top-level struct selection | |
| */ |
There was a problem hiding this comment.
I apologize that I didn't check it carefully. Adjusted It
There was a problem hiding this comment.
no worries. I seem to have given you a code suggestion that isn't formatted properly by accident and you would need to run spotlessApply to fix the indendation
There was a problem hiding this comment.
@nielspardon I executed spotlessApply task for lint
Co-authored-by: Niels Pardon <mail@niels-pardon.de>
Summary
Implement the
MaskExpressionPOJO layer forReadRelprojection support in substrait-java.The Substrait protobuf spec defines
Expression.MaskExpressionas a field onReadRel(field 4:projection), but the Java POJO layer had only a TODO comment and no actual implementation. This means anyReadRelprojection information was silently dropped during proto-to-POJO conversion, and POJO-constructed plans could not express column projection at the read level.This change closes that gap by introducing a full POJO model for
MaskExpressionand wiring it through the existing conversion infrastructure.Changes
MaskExpression.java(new) --@Value.Enclosinginterface with 11@Value.Immutableinner types (MaskExpr,Select,StructSelect,StructItem,ListSelect,ListSelectItem,ListElement,ListSlice,MapSelect,MapKey,MapKeyExpression) matching the proto definition 1:1MaskExpressionProtoConverter.java(new) -- Dedicated bidirectional converter betweenio.substrait.proto.Expression.MaskExpressionand the POJO types, keeping proto FQN references isolated from the relation convertersAbstractReadRel.java-- Replaced TODO comment withOptional<MaskExpression.MaskExpr> getProjection()ProtoRelConverter.java-- AddedoptionalMaskExpression()helper; all fourReadRelsubtypes (NamedScan,LocalFiles,VirtualTableScan,ExtensionTable) now populate projection during proto-to-POJO conversionRelProtoConverter.java-- All fourvisit(ReadRel)methods now serialize projection back to proto viaMaskExpressionProtoConverter.toProto()Testing
Added 11 roundtrip tests in
ReadRelRoundtripTestcovering:StructSelectonly)StructItemwith childSelect.ofStruct)ListSelect)ListSelect+ childStructSelect)MapSelectwithMapKey)MapKeyExpression)MapSelect+ childStructSelect)maintainSingularStructflagVirtualTableScanandLocalFiles