feat(isthmus): support month()-style datetime operators #643
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mbwhite
force-pushed
the
fix-month-datetime
branch
3 times, most recently
from
c8844e9 to
0f67f0f
Compare
mbwhite
force-pushed
the
fix-month-datetime
branch
from
0f67f0f to
b963373
Compare
bestbeforetoday
requested changes
Dec 17, 2025
Member
bestbeforetoday
left a comment
bestbeforetoday
left a comment
There was a problem hiding this comment.
Generally looks good to me with just a few minor tweaks desirable.
isthmus/src/main/java/io/substrait/isthmus/expression/ExtractDateFunctionMapper.java
Outdated
Show resolved
Hide resolved
isthmus/src/main/java/io/substrait/isthmus/expression/ExtractDateFunctionMapper.java
Outdated
Show resolved
Hide resolved
isthmus/src/main/java/io/substrait/isthmus/expression/ExtractDateFunctionMapper.java
Outdated
Show resolved
Hide resolved
isthmus/src/main/java/io/substrait/isthmus/expression/LiteralConverter.java
Outdated
Show resolved
Hide resolved
isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java
Outdated
Show resolved
Hide resolved
isthmus/src/main/java/io/substrait/isthmus/expression/ScalarFunctionConverter.java
Outdated
Show resolved
Hide resolved
vbarua
reviewed
Dec 18, 2025
| _data | ||
| **/*/bin | ||
| build | ||
| substrait.plan |
Contributor
Author
There was a problem hiding this comment.
It's created for the examples, and I was used them as a test and kept adding it to the commit.
isthmus/src/main/java/io/substrait/isthmus/expression/EnumConverter.java
Outdated
Show resolved
Hide resolved
isthmus/src/main/java/io/substrait/isthmus/expression/EnumConverter.java
Outdated
Show resolved
Hide resolved
vbarua
changed the title
MONTH() style functions for calcite
mbwhite
force-pushed
the
fix-month-datetime
branch
from
224bff1 to
1bcd2f9
Compare
mbwhite
force-pushed
the
fix-month-datetime
branch
from
1bcd2f9 to
6a4d236
Compare
Contributor
Author
|
@vbarua @bestbeforetoday hopefully I've been able to address all your points. Thanks |
bestbeforetoday
requested changes
Dec 19, 2025
| @Value.Immutable | ||
| public interface EnumArg extends FunctionArg { | ||
|
|
||
| Logger LOGGER = LoggerFactory.getLogger(EnumArg.class); |
Member
There was a problem hiding this comment.
This is such a minor issue I pushed an additional commit to fix this so the PR doesn't need to be held out until you change it.
The Substrait Function for Month style mappings doesn't work as the calcite only has 2 arguments, but substrait has three. Isthmus (in either direction) assumes that there is a 1:1 mapping betwen a substrait function and a calicte function. With the same number of arguments in each. (although types can be coerced if needeed) In this case, the 3 arg Substrait function needs to be mapped to a 2 arg calcite one. In effect this is working by swapping out the function to a different one and then letting the rest of the logic work as before. A concern is the increased complexity - and the code as a whole wasn't built with this in mind. Signed-off-by: MBWhite <whitemat@uk.ibm.com>
Signed-off-by: MBWhite <whitemat@uk.ibm.com>
Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
bestbeforetoday
force-pushed
the
fix-month-datetime
branch
from
6a4d236 to
99ab784
Compare
bestbeforetoday
approved these changes
Dec 19, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Substrait Function for Month style mappings doesn't work as the calcite only has 2 arguments, but substrait has three.
Isthmus (in either direction) assumes that there is a 1:1 mapping between a substrait function and a calcite function. With the same number of arguments in each. (although types can be coerced if needed)
In this case, the 3 arg Substrait function needs to be mapped to a 2 arg calcite one. In effect this is working by swapping out the function to a different one and then letting the rest of the logic work as before.
A concern is the increased complexity - and the code as a whole wasn't built with this in mind.