fix(isthmus)!: translate array indexes correctly

[bvolpato]

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.

SELECT a[1] FROM (VALUES (ARRAY[10, 20, 30])) AS t(a);

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, or offsets at or above the list-length limit, 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.

Downsides

Substrait list references cannot preserve the throwing behavior of unsafe OFFSET/ORDINAL operators. Those operators fall through to unsupported-function handling. Converting ordinary list selections back to Calcite remains unsupported.

This change does not alter the derived type of a list selection. The type stays the declared element type, so a selection that returns null can still derive a required type. The spec does not define selection type nullability. #1338 tracks that question for :core.

BREAKING CHANGE: Unsafe array access operators (OFFSET, ORDINAL) now fail conversion. Safe accesses such as a[0], negative indexes, NULL indexes, and indexes of 2^31 or more now return null; invalid indexes are no longer copied or narrowed into a potentially valid list offset. SAFE_OFFSET remains zero-based. Callers must handle unsupported unsafe operators and remove workarounds that depended on the previous invalid-index behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: substrait-io/substrait-java/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 6a3e7cd4-69b0-4a19-999f-1e5c42344ce3

---

---

📥 Commits

Reviewing files that changed from the base of the PR and between 09da9d6 and 0af3151.

---

---

📒 Files selected for processing (2)

• isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java
• isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java

---

---

🚧 Files skipped from review as they are similar to previous changes (1)

• isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java

---

---

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

---

---

---

📝 Summary

Summary by CodeRabbit

• Bug Fixes
  • Corrected array element selection so zero-based and one-based indexing operators resolve to the expected positions, including within nested lists.
  • Safe array indexing now handles null, invalid, and out-of-range indices consistently. Unsupported or unsafe indexing operators are not converted.
  • Preserved array operands and references when converting supported selections, including selections nested within collections.

📝 Summary

Walkthrough

Array field selection now accepts safe Calcite array operators with index bases 0 or 1. It converts supported indexes to Substrait list offsets. Null, below-base, and oversized indexes use Integer.MAX_VALUE. Tests cover nested selections and round trips.

Changes

Array Index Conversion

Layer / File(s)	Summary
Safe array index conversion isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java	The converter accepts safe ITEM operators with base 0 or 1. It parses indexes as long, adjusts valid indexes, caps offsets at Integer.MAX_VALUE, and uses that value for null or below-base indexes.
Array selection conversion tests isthmus/src/test/java/io/substrait/isthmus/expression/FieldSelectionConverterTest.java, isthmus/src/test/java/io/substrait/isthmus/NestedStructQueryTest.java	Tests cover operator bases, invalid and oversized indexes, operand preservation, nested selections, and round trips. Nested query expectations use zero-based offsets.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: nielspardon

---

---

Merge Risk: ⚪ Minimal · up to 0af31

The change updates array-index offsets, and the nested-query tests check the expected zero-based offsets. No concrete user-facing failure is established, so the PR appears mergeable subject to normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 3424c

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

• Low · architecture · inferred: Null and below-base indexes are encoded as ordinary list offsets at Integer.MAX_VALUE. Their null-result guarantee depends on an unverified maximum-list-length contract. If a consumer permits an element at that offset, the translation could return data instead of null. This is an unresolved portability assumption, not a verified security finding.

Security review details

Security Blast Radius

• inferred — The demonstrated propagation is through translated query expressions, shared field-reference typing and protobuf consumers. The supplied evidence does not establish deployment-level tenant, service or data-store exposure.

Trust Boundaries and Controls

• observed — Literal-only indexing, safe-operator checks, supported-base checks and wide-offset validation constrain the array-reference translation. These checks limit semantic misinterpretation; they are not tenant authorization controls.

Resilience and Maintainability Implications

• observed — The throwing-operand test asserts that the original Calcite expression throws and that translation retains its operand tree. Additional tests check nullable types and protobuf round trips. These assertions support structural error preservation, but do not prove downstream execution behavior.

---

---

---

Pre-merge checks | 4 | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 4.65% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 43 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title is concise, uses valid Conventional Commit syntax, identifies the isthmus scope, and accurately describes the array-index translation fix. The breaking-change marker matches the documented b…
Description check		The description provides the rationale, technical behavior, downsides, unsupported operators, compatibility impact, and a clear BREAKING CHANGE footer. It is complete and aligned with the repository t…

---

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

---

---

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Comment @coderabbitai help to get the list of available commands.}

[nielspardon]

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).

[bvolpato]

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]

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.

[bvolpato]

Good catches. Unbound fields and map values now stay unchanged when the parent is required. Untyped NULL indexes bypass literal conversion, and oversized offsets use the out-of-range sentinel. I reproduced the required-schema virtual-table failure too. You're right that the core nullability change needs separate review; its compatibility with Go-produced plans is still open.

[nielspardon]

Please move the :core nullability commits out of this PR into a separate fix(core)! PR for #1338. Every remaining problem I found comes from them: plans with a list selection in a virtual table no longer load, ARRAY[ARRAY[a[1]], ARRAY[0]] no longer converts, and an identity RelCopyOnWriteVisitor pass retypes scans whose schema struct is nullable. None of that is needed for the index fix, and the isthmus side is close.

[bvolpato]

This PR now contains only the isthmus index translation. I removed the :core nullability commits (5affd6d), so the three regressions from them no longer apply: virtual-table plans with a list selection, ARRAY[ARRAY[a[1]], ARRAY[0]], and the identity RelCopyOnWriteVisitor pass. The description and the BREAKING CHANGE footer now cover only the offset translation and the rejected OFFSET/ORDINAL operators. The core change will go to a separate fix(core)! PR for #1338.

[nielspardon]

Thanks, the split and both fixes look good. Please correct the BREAKING CHANGE footer, since the release notes publish it verbatim: SAFE_OFFSET already wrote a zero-based offset, and what consumers will actually notice is that a[0], negative indexes, a NULL index and indexes of 2³¹ or more now return null instead of an element.

[bvolpato]

Thanks, I've addressed these. The footer now describes the invalid-index changes and makes clear that SAFE_OFFSET was already zero-based. The Javadoc says the index is translated, and the nested test checks each list offset explicitly. Pushed 0af31510; the full local build passes. Ready for another look.