Skip to content

Conversation

@asuriy
Copy link
Contributor

@asuriy asuriy commented Oct 23, 2025

  • Addition of the missing scalar function (sqrt) mapping from substrait to substrait-java

  • Mapped from https://github.com/substrait-io/substrait/blob/main/extensions/functions_arithmetic.yaml

  • Builds successfully with added test

  • Added line 67 to substrait-java/isthmus/src/main/java/io/substrait/isthmus/expression/FunctionMappings.java

  • Added to line 46 in substrait-java/isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java

  • Added substrait-java/isthmus/src/main/java/io/substrait/isthmus/expression/SqrtFunctionMapper.java

  • Added test to substrait-java/isthmus/src/test/java/io/substrait/isthmus/ArithmeticFunctionTest.java

asuriy and others added 2 commits October 24, 2025 22:29
The sqrt function does not require any custom mapping since the SQL and
Substrait function signatures are compatible. The default mapping
behavior can be used. Custom mapping is required only to convert
power(x, 0.5) to sqrt(x).

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
Copy link
Contributor

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

@asuriy I pushed an additional commit to your PR with suggested changes. The major change is the removal of the custom mapping for the sqrt function itself. The default mapping is sufficient here so the custom mapper can just ignore this case. Custom mapping is only required to convert power(x, 0.5) to sqrt(x). I made some very slight logical changes to (I think) make the intent clearer. Mainly around using isPowerOfHalf as a function name to check whether to apply custom mapping instead of the rather cryptic includesPower.

I also took the liberty of rebasing both of our commits on top of the current main branch content, removing the two merge commits you added on top of your commit. I would recommend git rebase on the remote main branch to get a pull request up-to-date instead of pulling merge commits. It keeps your PR history clean instead of intermingling commits from your branch and the upstream. Some repositories will not allow merge commits in pull requests because of this.

If you are happy with the change I have made, we could go ahead as-is. Or we could get somebody else to review it too.

@asuriy
Copy link
Contributor Author

asuriy commented Oct 24, 2025

@bestbeforetoday I've had a look at the code changes you have made and those are fair changes, thank you! Hopefully the more accustomed I get with the code base, the easier it is to spot what is sufficient. Happy to go ahead with your changes, thank you.

@bestbeforetoday bestbeforetoday merged commit a140d21 into substrait-io:main Oct 25, 2025
12 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.

2 participants