fix(isthmus)!: index extended expression fields against the combined schema

[alexandrefimov]

References to columns of later tables used indices local to their own table, while base_schema combines all tables. Build them in the order of the combined schema.

Closes #1200

BREAKING CHANGE: with more than one table, a column of any table after the first gets its index in base_schema. With A (A1, A2, A3) and B (B1, B2), B2 is field 4, not 1. This includes the repeatable -c/--create CLI option.

[nielspardon]

Retitle this fix(isthmus)!: and add a BREAKING CHANGE: footer naming the changed indices — the emitted field moves for every caller that passes more than one CREATE statement, and -c/--create is repeatable, so the CLI reaches it too. Only the title reaches CHANGELOG.md, so as titled this is invisible to downstreams, and #1161, #1169, #1171, #1189, #1151 and #1248 all carry ! plus a footer for exactly this shape. Also drop "following spec v0.102.0" from the body — v0.102.0 changed nothing about extended expressions, so the marker claims a spec change that did not happen.

The fix itself is right. The rest is inline: one design question about how the index is derived, plus test and doc suggestions.

[coderabbitai]

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 995fcec2-3348-4633-96b9-f493dc69300d

📥 Commits

Reviewing files that changed from the base of the PR and between fff6390 and 8c830d8.

📒 Files selected for processing (2)

• isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java
• isthmus/src/test/java/io/substrait/isthmus/SimpleExtendedExpressionsTest.java

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

The converter now assigns field references using each column’s position in the combined schema. Documentation describes the schema ordering and duplicate-name requirement. Tests cover indices across tables, conversions, function arguments, and round-tripped types.

Changes

Combined-schema field indexing

Layer / File(s)	Summary
Combined-schema indexing and validation isthmus/src/main/java/io/substrait/isthmus/SqlExpressionToSubstrait.java, isthmus/src/test/java/io/substrait/isthmus/SimpleExtendedExpressionsTest.java	The converter uses accumulated schema positions for field references. Its documentation describes the combined schema and duplicate-name exception. Tests check indices across table definitions, expressions, repeated conversions, and round-tripped types.

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

Suggested reviewers: nielspardon

Merge Risk: ⚪ Minimal · up to 8c830

References to columns in later tables now use their positions in the combined schema. No remaining issue in the supplied evidence prevents merging after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Issue #1200 requires field references to use positions in the combined base_schema and requires a schema-based test. The implementation sets each RexInputRef index from nameToTypeMap.size() befo…
Out of Scope Changes check	✅ Passed	The pull request changes only SqlExpressionToSubstrait and its focused SimpleExtendedExpressionsTest. The added documentation explains the required combined-schema behavior. The added tests suppor…
Title check	✅ Passed	The title is concise, specific, and directly describes the fix to index extended-expression fields against the combined schema. It uses a valid breaking Conventional Commit format.
Description check	✅ Passed	The description states the rationale, explains the behavior change with a concrete example, identifies the affected CLI option, references issue #1200, and includes the required BREAKING CHANGE footer…

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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

[nielspardon]

All six threads are addressed, and the two you kept as-is are the right calls — selectedField really does not transfer to SubqueryPlanTest, which asserts outer references rather than root ones. Nothing further from me.

[nielspardon]

LGTM