docs(isthmus): javadoc for io.substrait.isthmus function coverter classes#760
Open
mbwhite wants to merge 1 commit intosubstrait-io:mainfrom
Open
docs(isthmus): javadoc for io.substrait.isthmus function coverter classes#760mbwhite wants to merge 1 commit intosubstrait-io:mainfrom
mbwhite wants to merge 1 commit intosubstrait-io:mainfrom
Conversation
mbwhite
changed the title
Signed-off-by: MBWhite <whitemat@uk.ibm.com>
mbwhite
force-pushed
the
fix-javadoc-c-010
branch
from
f3d0d26 to
4306679
Compare
bestbeforetoday
requested changes
Mar 24, 2026
Member
bestbeforetoday
left a comment
bestbeforetoday
left a comment
There was a problem hiding this comment.
Looks generally good. A few inline comments that I think are worth addressing.
Comment on lines
-55
to
-60
| * <ul> | ||
| * <li><b>Calcite → Substrait:</b> Subclasses implement {@code convert()} methods to convert | ||
| * Calcite calls to Substrait function invocations | ||
| * <li><b>Substrait → Calcite:</b> {@link #getSqlOperatorFromSubstraitFunc} converts Substrait | ||
| * function keys to Calcite {@link SqlOperator}s | ||
| * </ul> |
Member
There was a problem hiding this comment.
I think that removing the information in this list lost some useful overview information on how subclass implementors should extend this abstract class effectively. I suggest keeping this information.
Comment on lines
-85
to
-87
| * <p>If there are multiple functions provided with the same name and signature (e.g., from | ||
| * different extension URNs), the last one in the list will be given precedence during Calcite to | ||
| * Substrait conversion. |
Member
There was a problem hiding this comment.
I found the more verbose original version slightly more explanatory. Particularly the example that functions signatures might be the same for functions defined in different extension URNs.
| /** SqlOperator. */ | ||
| public final SqlOperator operator; | ||
|
|
||
| /** Name. */ |
Member
There was a problem hiding this comment.
Suggested change
| /** Name. */ | |
| /** Substrait function name. */ |
| /** SqlOperator. */ | ||
| public final SqlOperator operator; | ||
|
|
||
| /** Types. */ |
Member
There was a problem hiding this comment.
Suggested change
| /** Types. */ | |
| /** Allowed output types. */ |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
No description provided.