Conversation
|
ACTION NEEDED Substrait follows the Conventional Commits The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
mbwhite
force-pushed
the
additional-substrait-builder
branch
from
bd62615 to
d0cc543
Compare
mbwhite
changed the title
mbwhite
force-pushed
the
additional-substrait-builder
branch
from
d0cc543 to
6c079f0
Compare
Signed-off-by: MBWhite <whitemat@uk.ibm.com>
mbwhite
force-pushed
the
additional-substrait-builder
branch
2 times, most recently
from
8618365 to
434df46
Compare
Signed-off-by: MBWhite <whitemat@uk.ibm.com>
mbwhite
force-pushed
the
additional-substrait-builder
branch
from
434df46 to
83fa865
Compare
| */ | ||
| public Expression.I32Literal i32(int v) { | ||
| return Expression.I32Literal.builder().value(v).build(); | ||
| public Expression.I32Literal i32(final int value) { |
There was a problem hiding this comment.
Do we need to alter all of the signatures in this PR as final? It's adding a lot of diff to this PR and IMO it doesn't really add anything extra in terms of guarantees.
There was a problem hiding this comment.
Adding final is something my editor is set to do, but happy to work with the project guidelines;
i32 and fp64 are the only two existing methods I've altered. Everything else is new (or newly public)
There was a problem hiding this comment.
Sounds good. I'm on my phone so can't look at the rest of the file, but can we only use final for new methods if it's consistent with the other methods then? Do the other ones mark every method as final?
mbwhite
force-pushed
the
additional-substrait-builder
branch
2 times, most recently
from
56aaa2a to
5bdace0
Compare
Signed-off-by: MBWhite <whitemat@uk.ibm.com>
mbwhite
force-pushed
the
additional-substrait-builder
branch
from
5bdace0 to
ef5e526
Compare
benbellick
left a comment
benbellick
left a comment
There was a problem hiding this comment.
Just a few comments. Thanks!
| * @param value value to create | ||
| * @return i16 instance | ||
| */ | ||
| public Expression.I8Literal i8(final int value) { |
There was a problem hiding this comment.
| public Expression.I8Literal i8(final int value) { | |
| public Expression.I8Literal i8(int value) { |
And yeah looking at these files on my computer now. It does look like we consistently don't use final in all of the other methods. Can we drop the usage of final in all of these places to keep our API consistent? That way we can have a separate conversation about introducing final if we want it.
| * @param input the input relation to aggregate | ||
| * @return an Aggregate relation representing the grouping and aggregation operation | ||
| */ | ||
| public Aggregate aggregate( |
There was a problem hiding this comment.
Sorry, I'm a bit confused here. Now that this method is made public, what is the difference between this one and the above one? Just that Rel.Remap is not required, but is instead optional?
Can we instead just either a) have one aggregate builder in which the Remap is marked optional, or b) have two different impls, one in which the Remap is required, and the other in which there is no Remap argument?
Otherwise, its a bit unclear to me which one users are supposed to use.
There was a problem hiding this comment.
I think my preference for now is just the one singular one where it is marked optional. This would technically be a breaking change but would be a minimal diff.
2 participants
Follow on to pr #767 to fill some missing gaps in the substrait builder. We've recently used the Substrait Builder and found that there some variants and missing gaps in the APIs. We'd subclassed it to add these, so I'm now wanting to push these to the main codebase.
There wasn't a unit test already so I've added a basic one here - which I appreciate will is quite large.