Conversation
8659726 to
125c546
Compare
Normalize safe array indexes by their operator base. Preserve null and out-of-range low indexes without introducing negative list offsets, and check wide indexes before narrowing. BREAKING CHANGE: Unsafe array operators and unrepresentable offsets now reach unsupported-function handling instead of changing semantics.
Folding an invalid or null index to NULL can remove a throwing cast or another computed array operand. Restrict this fold to literals and references into an existing record; reject other operands when their evaluation cannot be preserved. Cover throwing casts, nested array selections, computed operands, and safe literal and column references for ITEM, SAFE_OFFSET, and SAFE_ORDINAL.
125c546 to
bafd921
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 47 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (7)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughField references now reflect container nullability, and list selections return nullable element types. Calcite array selection conversion now handles supported ChangesArray Selection and Field Nullability
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change fixes array index translation for safe Calcite operators and makes field-reference nullability more accurate. No concrete merge-blocking problem was identified. The nullability change may affect downstream code that assumed non-null element types. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected changes preserve operand expressions and reject unsupported array operators and oversized offsets. However, invalid-index handling depends on a list-size guarantee that remains unverified across consumers. No security vulnerability or expanded privilege boundary was established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 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.
Thanks, the offset translation itself looks right. Before a line-by-line pass I'd like to settle one design question on the NULL-folding path (inline).
Use a guaranteed out-of-range Substrait list offset for null and below-base indexes, retaining computed operand evaluation and collection composition. Infer nullable list-selection results consistently through nested references and protobuf conversion. BREAKING CHANGE: List selections now report nullable element types, and selections through nullable struct or map parents propagate that nullability.
Merge current main and reuse the field-type finders in nonthrowing reference resolution, preserving bounds and map-key validation. Cover nullable parents and nested list selections, and update the retained reference expectation to its nullable type.
|
I went with the reference approach so the array operand is still evaluated. The spec’s list-length limit makes the chosen positive offset out of range. Nullable result types now survive nested selections and protobuf conversion, including the collection, ROW, and UDT cases you raised. |
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Thanks, the out-of-range offset works: the spec caps lists at 2,147,483,647 elements, so no element can be at that offset. Please move the :core nullability change into its own fix(core)! PR for this one to stack on. It changes derived types for every core user, only this PR's title reaches the changelog, and the spec doesn't say how selection nullability should work, so that PR should say so.
| } | ||
| return expr.fields().get(index); | ||
| Type field = expr.fields().get(index); | ||
| return field.withNullable(field.nullable() || expr.nullable()); |
There was a problem hiding this comment.
Only widen when the parent is nullable, and do the same for map values at line 651. Type.Unbound.nullable() throws, so selecting an unbound field now crashes, e.g. in an identity RelCopyOnWriteVisitor pass over a scan with an unbound column.
| return field.withNullable(field.nullable() || expr.nullable()); | |
| return expr.nullable() ? field.withNullable(true) : field; |
| public Type visit(Type.ListType expr) throws RuntimeException { | ||
| return expr.elementType(); | ||
| // An out-of-range offset returns null even when the list's elements are required. | ||
| return expr.elementType().withNullable(true); |
There was a problem hiding this comment.
Decide how VirtualTableScan's exact row-type check (VirtualTableScan.java:95) should handle this. A virtual-table row holding list_element{0} over list<i32> against a REQUIRED i32 schema column now fails ProtoRelConverter.from. substrait-go builds plans of that shape.
| return Optional.empty(); | ||
| } | ||
| long offset = Integer.MAX_VALUE; | ||
| if (!(literal instanceof Expression.NullLiteral)) { |
There was a problem hiding this comment.
Check ((RexLiteral) reference).isNull() before line 76 converts the literal. An untyped a[NULL] throws Unable to convert the type NULL there, so only a[CAST(NULL AS INTEGER)] reaches this branch.
| if (index.get() >= operator.offset) { | ||
| offset = index.get() - operator.offset; | ||
| } | ||
| } | ||
| if (offset > Integer.MAX_VALUE) { | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
Clamp instead of rejecting: any offset of 2,147,483,647 or more selects nothing, so a[3000000000] can use the same out-of-range offset. unrepresentableOffsetsAreRejected and the "Downsides" sentence then need to change too.
| if (index.get() >= operator.offset) { | |
| offset = index.get() - operator.offset; | |
| } | |
| } | |
| if (offset > Integer.MAX_VALUE) { | |
| return Optional.empty(); | |
| } | |
| if (index.get() >= operator.offset) { | |
| offset = Math.min(index.get() - operator.offset, Integer.MAX_VALUE); | |
| } | |
| } |
2 participants
Calcite's safe array operators use zero- or one-based indexes, while Substrait list references use zero-based offsets and interpret negative offsets from the end. Copying the literal index changes the selected value: for a = [10, 20, 30], a[1] becomes the second element and a[-1] becomes the last instead of null.
Normalize indexes using each operator's declared base and check wide integer indexes before narrowing to the protobuf offset. For null or below-base indexes, retain the list reference with offset 2,147,483,647. The Substrait list-length limit makes that offset out of range, so the result is null while the array operand is still evaluated.
Derive list selections as nullable even when their elements are required, and propagate nullable parents through subsequent struct and map selections. This preserves nullable result types through protobuf conversion and avoids folding null accesses into malformed collection literals or losing computed-operand evaluation.
Downsides
Substrait list references cannot preserve the throwing behavior of unsafe OFFSET/ORDINAL operators or represent offsets outside the signed 32-bit range. These conversions fall through to unsupported-function handling. Converting ordinary list selections back to Calcite remains unsupported.
Summary by CodeRabbit
SAFE_OFFSETandSAFE_ORDINAL, into Substrait field selections. Valid indexes are converted to zero-based offsets, including indexes represented by larger numeric values.BREAKING CHANGE: Unsafe array access operators and unrepresentable indexes now fail conversion. List-selection result types are nullable, and subsequent struct or map selections inherit nullable parents. Callers must handle unsupported operators or indexes and use the resulting nullable types instead of assuming required elements imply a required selection result.