Skip to content

Commit

Permalink
fix: set proto-to-rel type nullability handling
Browse files Browse the repository at this point in the history
  • Loading branch information
Blizzara committed Mar 5, 2025
1 parent fb36197 commit 2945e97
Showing 1 changed file with 36 additions and 1 deletion.
37 changes: 36 additions & 1 deletion core/src/main/java/io/substrait/relation/Set.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

import io.substrait.proto.SetRel;
import io.substrait.type.Type;
import io.substrait.type.TypeCreator;
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Stream;
import org.immutables.value.Value;

@Value.Immutable
Expand Down Expand Up @@ -42,7 +46,38 @@ public static Set.SetOp fromProto(SetRel.SetOp proto) {

@Override
protected Type.Struct deriveRecordType() {
return getInputs().get(0).getRecordType();
// The different inputs may have schemas that differ in nullability, but not in type.
// In that case we should return a schema that is nullable where any of the inputs is nullable.
Stream<Type.Struct> inputRecordTypes = getInputs().stream().map(Rel::getRecordType);
return inputRecordTypes
.reduce(
(a, b) -> {
if (a.equals(b)) {
return a; // Short-circuit trivial case
}

if (a.fields().size() != b.fields().size()) {
throw new IllegalArgumentException(
"Set's input records have different number of fields");
}

List<Type> fields = new ArrayList<>();
for (int i = 0; i < a.fields().size(); i++) {
// We intentionally don't validate that the types match, to maintain backwards
// compatibility. We should, but then we'll need to handle things like VARCHAR
// vs FIXEDCHAR (comes up in Isthmus tests). We also don't recurse into nullability
// of the inner fields, in case the type itself is a struct or list or map.
Type typeA = a.fields().get(i);
Type typeB = b.fields().get(i);
fields.add(!typeB.nullable() ? TypeCreator.asNullable(typeA) : typeA);
}

return Type.Struct.builder()
.fields(fields)
.nullable(a.nullable() || b.nullable())
.build();
})
.orElseThrow(() -> new IllegalStateException("A Set relation needs at least one input"));
}

@Override
Expand Down

0 comments on commit 2945e97

Please sign in to comment.