fix(isthmus)!: keep a struct's declared field nullability in Calcite - #1317
nielspardon merged 3 commits into
Conversation
A Substrait struct declares its own nullability and its fields' separately. TypeConverter applied the struct's with createTypeWithNullability, which Calcite documents as widening a record's fields along with the record, so struct?<i32, fp64> came back as struct?<i32?, fp64?>. Apply it with enforceTypeWithNullability, which the factory canonizes the same way but which leaves each field as declared. A computed field inside a nullable struct now converts back instead of reporting that the row type cannot say what the schema said. Closes substrait-io#1154
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change preserves declared field nullability when converting nullable Substrait structs to Calcite. It also separates row nullability from field nullability and updates output-name matching for nested types. ChangesStruct conversion and output names
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established for the struct-nullability and output-name changes; the PR is ready for normal merge checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The conversion behavior is a breaking contract change, but the inspected paths retain structural checks before applying output names and show no new privileged action. Effects on downstream consumers remain uncertain. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Normalize the collected row type's outermost struct to NOT NULL in SchemaCollector (isthmus/src/main/java/io/substrait/isthmus/SchemaCollector.java:73-75), the way the virtual-table path already does. Without it a nullable schema struct now produces a Calcite row type that is a nullable record with NOT NULL columns, and since createTypeWithNullability returns a type unchanged when the nullability already matches, Calcite's own outer-join widening becomes a no-op — a LEFT join's null-generating side keeps columns that Join.getRecordType() calls nullable. proto/substrait/type.proto says a relation schema's outermost struct should be NULLABILITY_REQUIRED, so this is also what the schema was supposed to be.
// A relation's row type says what its columns are, not whether a value is there, and
// Calcite builds one NOT NULL wherever it derives one -- so a schema struct contributes
// its fields and not its own nullability.
RelDataType rowType =
typeFactory.createTypeWithNullability(
typeConverter.toCalcite(typeFactory, namedStruct.struct(), namedStruct.names()),
false);A nullable schema struct became a nullable Calcite record with NOT NULL columns. createTypeWithNullability returns such a type unchanged when asked for nullable, so outer-join widening kept the null-generating side's columns NOT NULL. Normalize the outermost struct as the virtual-table path already does; type.proto asks for a required outermost struct anyway.
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Make describesColumnsOf compare a struct column field by field, ignoring nullability (isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java:1289-1293) — its two-arg equalSansNullability compares the whole type string, so now that a nullable struct's fields keep NOT NULL, a Project whose struct column comes from an if_then silently drops its hint's output names ([renamed] on main, [s0] here). The same change fixes #1214, so landing it there instead works too.
RelDataType declared = typeConverter.toCalcite(typeFactory, fields.get(field));
RelDataType actual = rowType.getFieldList().get(field).getType();
// A struct column is compared field by field, ignoring each field's nullability and name:
// only the top-level names are restated here, and the declared type is converted without a
// name list, so the names inside it are placeholders.
boolean describes =
declared.isStruct() && actual.isStruct()
? SqlTypeUtil.equalAsStructSansNullability(typeFactory, declared, actual, null)
: SqlTypeUtil.equalSansNullability(declared, actual);
if (!describes) {
return false;
}…ility describesColumnsOf compared a struct column's whole type, nested names and field nullability included. The declared type converts without a name list, so a column holding a struct never matched and the hint's names were dropped. Now that a nullable struct keeps its fields' nullability, a struct computed by an if_then lost its names the same way. Types are now compared ignoring names and nullability at every depth, through structs, lists and maps. Also restates the comments that described the removed widening, adds full round trips to the nullable struct virtual-table tests, and pins a nested struct in CalciteTypeTest.
2 participants
A Substrait struct declares its own nullability and its fields' separately.
TypeConverterapplied the struct's withcreateTypeWithNullability, which Calcite documents as widening a record's fields along with the record, sostruct?<i32, fp64>came back asstruct?<i32?, fp64?>.enforceTypeWithNullabilitycanonizes the result the same way and leaves each field as declared. A computed field inside a nullable struct now converts back instead of reporting that the row type cannot say what the schema said.SchemaCollectornow gives a table a NOT NULL row type, as a virtual table already has; otherwise a LEFT join did not widen its columns.A hint's output names now reach a column that holds a struct:
describesColumnsOfcompares the declared and converted types ignoring names and nullability at every depth.Summary by CodeRabbit
Closes #1154
Closes #1214
BREAKING CHANGE: Substrait-to-Calcite conversion keeps a struct field's declared nullability, so a required field inside a nullable struct stays NOT NULL.
SchemaCollectorgives a table a NOT NULL row type.