Skip to content

fix(isthmus): support variadic concat conversion#1001

Merged
nielspardon merged 1 commit into
substrait-io:mainfrom
bvolpato:bvolpato/fix-variadic-concat
Jul 13, 2026
Merged

fix(isthmus): support variadic concat conversion#1001
nielspardon merged 1 commit into
substrait-io:mainfrom
bvolpato:bvolpato/fix-variadic-concat

Conversation

@bvolpato

Copy link
Copy Markdown
Member

Summary

Convert variadic Substrait concat expressions into left-associated binary Calcite calls. This preserves argument order while satisfying Calcite || operand-count validation.

Closes #724.

Changes

  • Fold concat calls with more than two arguments into nested binary calls.
  • Keep existing two-argument conversion unchanged.
  • Add regression coverage for operator shape and operand order.

Repro

Converting concat("a", "b", "c") previously passed three operands to Calcite || and failed with wrong operand count 3 for ||. It now produces concat(concat("a", "b"), "c").

Testing

  • ./gradlew :isthmus:test --tests io.substrait.isthmus.FunctionConversionTest
  • ./gradlew spotlessApply
  • ./gradlew build --rerun-tasks

Local integrationTest could not start because no Docker daemon is available; GitHub Actions runs that Testcontainers suite.

Did this cause any problems?

No. Revert this commit to restore previous conversion behavior.

Nest Substrait concat arguments into binary Calcite calls while preserving operand order.

Closes substrait-io#724.

@nielspardon nielspardon 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, the only issue this introduces is that we can not cleanly round-trip plans Substrait -> Calcite -> Subtrait if the initial Substrait plan contained a variadic concat function call. I don't think there is an elegant way to solve this during conversion and maybe should be considered in the semantic-aware plan comparison issue #994

@nielspardon nielspardon merged commit 91bb733 into substrait-io:main Jul 13, 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.

Substrait string concat with more than two arguments fails conversion to Calcite

2 participants