Skip to content

feat(isthmus): observe field reference types - #1080

Merged
vbarua merged 3 commits into
substrait-io:mainfrom
alexandrefimov:issue-379-field-reference-type-observer
Aug 14, 2026
Merged

feat(isthmus): observe field reference types#1080
vbarua merged 3 commits into
substrait-io:mainfrom
alexandrefimov:issue-379-field-reference-type-observer

Conversation

@alexandrefimov

Copy link
Copy Markdown
Contributor

Summary

  • report Calcite input-schema types for root, outer and lambda field references through TypeObserver
  • track input row types and lambda parameter types in the conversion context
  • report a field reference converted with no input in scope as an inference failure instead of failing the conversion

Motivation

#379 lists four conversion points where the Substrait-supplied type and the type Calcite would infer can diverge; scalar functions (#1015) and window functions (#1059) are already observed. Field references are the case the issue singles out as needing access to the underlying input schema, which is what the scope tracking here adds.

Changes

Aggregate, sort and fetch open a conversion scope so their expressions can resolve input field types. Calcite's Aggregate and Sort carry no variablesSet, so an outer reference bound to one of those inputs has nowhere to be declared and is rejected — previously it failed anyway, in requireScope, because no scope existed at all. NamedUpdate applies its filter with the correlation ids resolved while converting the condition and the transformations.

Related to #379.

Report Calcite input-schema types for root, outer and lambda field references
through TypeObserver while keeping RexNode construction unchanged. The
conversion context now tracks input row types and lambda parameter types;
a field reference converted with no input in scope is reported as an
inference failure rather than failing the conversion.

Aggregate, sort and fetch open a conversion scope so their expressions can
resolve input field types. Calcite's Aggregate and Sort carry no
variablesSet, so a correlation bound to one of those inputs is rejected
instead of being silently dropped. Named update now applies its filter with
the correlation ids resolved while converting the condition and the
transformations.
@alexandrefimov
alexandrefimov force-pushed the issue-379-field-reference-type-observer branch from 2fc101f to fc0c796 Compare August 10, 2026 20:06
@alexandrefimov

Copy link
Copy Markdown
Contributor Author

Hi @vbarua, when you have a chance, could you take a look at the next #379 slice? It extends TypeObserver coverage from scalar and window functions to root, outer, and lambda field references. The main review point is whether the input and lambda scope tracking is the right boundary for observing Calcite-inferred field types. All 13 checks are green. Thanks!

@vbarua
vbarua self-requested a review August 11, 2026 15:04

@eatthepear eatthepear left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there, I work with @vbarua. It's been a while since I've looked at substrait-java, but I left a few comments of some things I noticed. Will leave the final approval for @vbarua

Comment thread isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java Outdated
Comment thread isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java Outdated

@bvolpato bvolpato left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will defer to Victor/Justin for a final word here though!

@alexandrefimov

Copy link
Copy Markdown
Contributor Author

Heads-up: the two OSV failures come from the existing Calcite/Avatica dependency chain (httpcore5 5.3.5), not from this change. The advisory was reviewed on August 12, so rerunning the jobs will not help. I’d keep the dependency update in a separate PR.

@alexandrefimov

Copy link
Copy Markdown
Contributor Author

The dependency fix is now in #1089. All three OSV checks are green there.

…ence-type-observer

# Conflicts:
#	isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java

@vbarua vbarua left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks everyone!

@vbarua
vbarua merged commit 45d64e1 into substrait-io:main Aug 14, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants