Skip to content

feat(isthmus): observe scalar function types - #1015

Merged
vbarua merged 1 commit into
substrait-io:mainfrom
alexandrefimov:issue-379-scalar-type-observer
Aug 4, 2026
Merged

feat(isthmus): observe scalar function types#1015
vbarua merged 1 commit into
substrait-io:mainfrom
alexandrefimov:issue-379-scalar-type-observer

Conversation

@alexandrefimov

Copy link
Copy Markdown
Contributor

Summary

  • add an opt-in type observer for Substrait-to-Calcite scalar function conversion
  • report the source expression, supplied Substrait type, and either the independently inferred Calcite type or the inference failure
  • expose observer configuration through ConverterProvider while keeping the default conversion path unchanged

Related to #379.

Behavior

  • observations are emitted for both matching and differing scalar function types
  • the converted RexNode continues to use the type supplied by Substrait
  • Calcite inference failures are reported without turning an otherwise successful conversion into a failure
  • exceptions thrown by the observer are propagated to the caller
  • variadic CONCAT produces one observation for the complete invocation

This PR intentionally covers scalar functions only. Window functions, field references, and aggregate functions remain separate follow-up work.

Validation

  • ./gradlew build --rerun-tasks
  • focused scalar observation tests covering matches, mismatches, inference failures, nested calls, provider injection, observer exceptions, and variadic CONCAT

Assisted-by: gpt-5.6-sol (OpenAI Codex)

@CLAassistant

CLAassistant commented Jul 17, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@nielspardon
nielspardon requested a review from vbarua July 20, 2026 12:11
@nielspardon

Copy link
Copy Markdown
Member

@vbarua can you take a look since you originally opened #379?

@vbarua

vbarua commented Jul 22, 2026

Copy link
Copy Markdown
Member

Did a quick pass and the direciton seems reasonable overall. Planning on taking a closer look tomorrow.

Comment thread isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java Outdated

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

Left some minor comments, but looks good overall.

Comment thread isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java Outdated
Comment thread isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java Outdated
Comment thread isthmus/src/main/java/io/substrait/isthmus/expression/TypeObservation.java Outdated
Comment thread isthmus/src/test/java/io/substrait/isthmus/SubstraitExpressionConverterTest.java Outdated
@alexandrefimov

Copy link
Copy Markdown
Contributor Author

Thanks for the review!
The five minor comments are clear.
I’ll address them together with the CONCAT simplification after #1025 is merged.

@vbarua

vbarua commented Jul 24, 2026

Copy link
Copy Markdown
Member

CONCAT simplification is merged 🙂

Assisted-by: gpt-5.6-sol (OpenAI Codex)
@alexandrefimov
alexandrefimov force-pushed the issue-379-scalar-type-observer branch from bca194f to ebbfe8a Compare July 27, 2026 08:49
@alexandrefimov

Copy link
Copy Markdown
Contributor Author

@vbarua rebased on main and addressed all review comments. Ready for another look when you have a chance. Thanks!

@alexandrefimov

Copy link
Copy Markdown
Contributor Author

@vbarua, gentle follow-up when you have a chance :)

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

Changes look good. Thanks for work on this 🙇

@vbarua
vbarua merged commit 243be42 into substrait-io:main Aug 4, 2026
13 checks passed
@vbarua

vbarua commented Aug 4, 2026

Copy link
Copy Markdown
Member

@alexandrefimov as a follow-up, if you wanted to make it easier to use the observer you've added, you could add it as a configurable setting in ConverterProvider.Builder

/**
* Fluent builder for {@link ConverterProvider}.
*
* <p>Unset components fall back to reasonable system defaults. The scalar, aggregate and window
* function converters, if not set explicitly, are derived from the configured {@link
* #extensions(SimpleExtension.ExtensionCollection) extensions} and {@link
* #typeFactory(RelDataTypeFactory) type factory} when {@link #build()} is called.
*/
public static class Builder {

@alexandrefimov

Copy link
Copy Markdown
Contributor Author

Thanks!

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