From f7cb019b60d365a0f9c1bf1674debe1698ac23ad Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Wed, 22 Jul 2026 20:08:12 +0200 Subject: [PATCH] fix(isthmus): use loose round-trip comparison for statistical aggregate integer inputs The round-trip helper that StatisticalFunctionTest's integer-input cases relied on was removed, leaving the isthmus test module uncompilable on main. Those cases cannot use the standard assertFullRoundTrip: the statistical- aggregate cast rewrite stacks a cast projection over the aggregate's input projection, which Calcite merges (and prunes the redundant column) when it rebuilds the plan on the Substrait -> Calcite leg. The plan straight from SQL therefore differs from the one after a Calcite -> Substrait -> Calcite round trip. Switch them to the existing assertSqlSubstraitRelRoundTripLoosePojoComparison helper, which is documented for exactly this optimizer-normalization case: it asserts that the normalized plan is stable across a round trip rather than requiring equality with the initial plan. --- .../substrait/isthmus/StatisticalFunctionTest.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/isthmus/src/test/java/io/substrait/isthmus/StatisticalFunctionTest.java b/isthmus/src/test/java/io/substrait/isthmus/StatisticalFunctionTest.java index 7dd03b5f5..286ebfe6f 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/StatisticalFunctionTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/StatisticalFunctionTest.java @@ -37,14 +37,16 @@ void roundTrip(String fn) throws Exception { } // Integer arguments are cast to fp64 (and the result cast back) since std_dev/variance only have - // fp32/fp64 signatures. This rewrite (castStatisticalAggregatesToFloatingPoint) inserts a cast - // projection that Calcite normalizes (project merge/column pruning) on the first round trip, so - // these use the identity-projection workaround, which asserts stability after normalization. + // fp32/fp64 signatures. The cast rewrite stacks a cast projection over the aggregate's input + // projection, which Calcite merges (and prunes the redundant column) when it rebuilds the plan on + // the Substrait -> Calcite leg. The plan straight from SQL therefore differs from the one after a + // Calcite -> Substrait -> Calcite round trip, so these compare loosely: they assert stability of + // the normalized plan rather than equality with the initial plan. @ParameterizedTest @CsvSource({"STDDEV_POP", "STDDEV_SAMP", "VAR_POP", "VAR_SAMP"}) void roundTripIntegerInput(String fn) throws Exception { - assertFullRoundTripWithIdentityProjectionWorkaround( + assertSqlSubstraitRelRoundTripLoosePojoComparison( String.format("SELECT %s(i32) FROM numbers", fn), SubstraitCreateStatementParser.processCreateStatementsToCatalog(CREATES)); } @@ -53,14 +55,14 @@ void roundTripIntegerInput(String fn) throws Exception { void roundTripIntegerInputSharedWithOtherAggregate() throws Exception { // The integer column is shared by SUM (which must keep operating on the integer) and STDDEV_POP // (which is cast to fp64); the cast must be appended, not applied in place. - assertFullRoundTripWithIdentityProjectionWorkaround( + assertSqlSubstraitRelRoundTripLoosePojoComparison( "SELECT SUM(i32), STDDEV_POP(i32) FROM numbers", SubstraitCreateStatementParser.processCreateStatementsToCatalog(CREATES)); } @Test void roundTripIntegerInputWithGrouping() throws Exception { - assertFullRoundTripWithIdentityProjectionWorkaround( + assertSqlSubstraitRelRoundTripLoosePojoComparison( "SELECT i8, VAR_POP(i32) FROM numbers GROUP BY i8", SubstraitCreateStatementParser.processCreateStatementsToCatalog(CREATES)); }