fix(isthmus): allow for conversion of plans containing Calcite aggregate functions #230
Conversation
This file contains 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
bvolpato
force-pushed
the
aggregate-function-converter-calcite-instance
branch
from
bc32c60 to
f7a2a65
Compare
|
FYI Check failures are unrelated to the change: |
bvolpato
force-pushed
the
aggregate-function-converter-calcite-instance
branch
from
f7a2a65 to
2139cff
Compare
bvolpato
changed the title
bvolpato
force-pushed
the
aggregate-function-converter-calcite-instance
branch
from
5501161 to
b9855de
Compare
vbarua
force-pushed
the
aggregate-function-converter-calcite-instance
branch
from
b9855de to
0e29574
Compare
vbarua
changed the title
vbarua
force-pushed
the
aggregate-function-converter-calcite-instance
branch
from
8201204 to
6c0c6fd
Compare
vbarua
reviewed
Feb 22, 2024
| return Optional.of( | ||
| fun.getKind() == SqlKind.MIN ? AggregateFunctions.MIN : AggregateFunctions.MAX); | ||
| } else if (aggFunction instanceof SqlAvgAggFunction) { | ||
| return Optional.of(AggregateFunctions.AVG); | ||
| } else if (aggFunction instanceof SqlSumAggFunction) { | ||
| return Optional.of(AggregateFunctions.SUM); | ||
| } else if (aggFunction instanceof SqlSumEmptyIsZeroAggFunction) { | ||
| return Optional.of(AggregateFunctions.SUM0); | ||
| } else { | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
Re-ordered the checks to match the function definitions above.
vbarua
approved these changes
Feb 22, 2024
| AggregateFunctions.getSubstraitAggVariant(aggFunction).orElse(aggFunction); | ||
| // Replace default Calcite aggregate calls with Substrait specific variants. | ||
| // See toSubstraitAggVariant for more details. | ||
| AggregateFunctions.toSubstraitAggVariant(aggFunction).orElse(aggFunction); |
There was a problem hiding this comment.
Added more detailed docs to the function definition
| public class OptimizerIntegrationTest extends PlanTestBase { | ||
|
|
||
| @Test | ||
| void conversionHandlesBuiltInSum0CallAddedByRule() throws SqlParseException, IOException { |
There was a problem hiding this comment.
This test captures how the default Calcite aggregate calls can be introduced by rules and verifies the conversion executes without failing even when this occur (thanks to your changes ✨ )
| } | ||
|
|
||
| final SubstraitBuilder b = new SubstraitBuilder(EXTENSION_COLLECTION); | ||
| public class AggregateFunctionConverterTest extends PlanTestBase { |
There was a problem hiding this comment.
Re-using code from PlanTestBase.
| @@ -55,5 +30,6 @@ public void testFunctionFinderMatch() { | |||
| null)); | |||
| assertNotNull(functionFinder); | |||
| assertEquals("sum0", functionFinder.getName()); | |||
| assertEquals(AggregateFunctions.SUM0, functionFinder.getOperator()); | |||
There was a problem hiding this comment.
Comparing the name is insufficient as both SqlSumEmptyIsZeroAggFunction and SubstraitSumEmptyIsZeroAggFunction have the same name.
vbarua
force-pushed
the
aggregate-function-converter-calcite-instance
branch
from
6c0c6fd to
09b6b2f
Compare
It looks good to me! Thanks for all the improvements / suggestions! |
ajegou
pushed a commit
to ajegou/substrait-java
that referenced
this pull request
Mar 29, 2024
…Functions (substrait-io#230) Calcite planning rules can introduce the Calcite variants of SqlAggFunctions In substrait-io#180, Substrait specific variants for these function were introduced which better matched the type inference for these functions as defined in Substrait Those changes cause failures when converting the Calcite variants to Substrait, which is what these changes address --------- Co-authored-by: Victor Barua <victor.barua@datadoghq.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Some of the Calcite CoreRules might bring their own functions at a late stage, so I've decided to make the converter flexible enough and handle the superclass types correctly (since we already do a similar conversion to
APPROX_COUNT_DISTINCTin the same spot).Another alternative would be to improve the matchers and check if names match or if it's an
instanceofthe passing call -- todaysignaturesis anIdentityHashMap, so it's really strict.