test: simplify tests and add special test for overlapping default ext

Added a test for ltrim to ensure there is no issue specifically with
the default extension collection functions (which have some special
handling in isthmus) and an introduced ltrim function.

Author: benbellick

isthmus/src/test/java/io/substrait/isthmus/DuplicateFunctionUrnTest.java

@@ -16,6 +16,7 @@
 import org.apache.calcite.rex.RexCall;
 import org.apache.calcite.rex.RexNode;
 import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.calcite.sql.fun.SqlTrimFunction.Flag;
 import org.junit.jupiter.api.Test;
 
 /** Tests to reproduce #562 */
@@ -33,21 +34,22 @@ public class DuplicateFunctionUrnTest extends PlanTestBase {
       collection2 = SimpleExtension.load("urn2://functions", extensions2);
       collection = collection1.merge(collection2);
 
-      // Verify that the merged collection contains duplicate functions with different URNs
+      // Verify that the merged collection contains duplicate concat functions with different URNs
       // This is a precondition for the tests - if this fails, the tests don't make sense
-      List<SimpleExtension.ScalarFunctionVariant> ltrimFunctions =
-          collection.scalarFunctions().stream().filter(f -> f.name().equals("ltrim")).toList();
+      List<SimpleExtension.ScalarFunctionVariant> concatFunctions =
+          collection.scalarFunctions().stream().filter(f -> f.name().equals("concat")).toList();
 
-      if (ltrimFunctions.size() != 2) {
+      if (concatFunctions.size() != 2) {
         throw new IllegalStateException(
-            "Expected 2 ltrim functions in merged collection, but found: " + ltrimFunctions.size());
+            "Expected 2 concat functions in merged collection, but found: "
+                + concatFunctions.size());
       }
 
-      String urn1 = ltrimFunctions.get(0).getAnchor().urn();
-      String urn2 = ltrimFunctions.get(1).getAnchor().urn();
+      String urn1 = concatFunctions.get(0).getAnchor().urn();
+      String urn2 = concatFunctions.get(1).getAnchor().urn();
       if (urn1.equals(urn2)) {
         throw new IllegalStateException(
-            "Expected different URNs for the two ltrim functions, but both were: " + urn1);
+            "Expected different URNs for the two concat functions, but both were: " + urn1);
       }
     } catch (IOException e) {
       throw new UncheckedIOException(e);
@@ -92,21 +94,21 @@ void testMergeOrderDeterminesFunctionPrecedence() {
         new ScalarFunctionConverter(reverseCollection.scalarFunctions(), typeFactory);
 
     RexBuilder rexBuilder = new RexBuilder(typeFactory);
-    RexNode arg1 = rexBuilder.makeLiteral("hello");
-    RexNode arg2 = rexBuilder.makeLiteral("world");
-    RexCall concatCall = (RexCall) rexBuilder.makeCall(SqlStdOperatorTable.CONCAT, arg1, arg2);
+    RexCall concatCall =
+        (RexCall)
+            rexBuilder.makeCall(
+                SqlStdOperatorTable.CONCAT,
+                rexBuilder.makeLiteral("hello"),
+                rexBuilder.makeLiteral("world"));
 
     // Create a simple topLevelConverter that converts literals to Substrait expressions
     java.util.function.Function<RexNode, Expression> topLevelConverter =
         rexNode -> {
-          if (rexNode instanceof org.apache.calcite.rex.RexLiteral) {
-            org.apache.calcite.rex.RexLiteral lit = (org.apache.calcite.rex.RexLiteral) rexNode;
-            return Expression.StrLiteral.builder()
-                .value(lit.getValueAs(String.class))
-                .nullable(false)
-                .build();
-          }
-          throw new UnsupportedOperationException("Only literals supported in test");
+          org.apache.calcite.rex.RexLiteral lit = (org.apache.calcite.rex.RexLiteral) rexNode;
+          return Expression.StrLiteral.builder()
+              .value(lit.getValueAs(String.class))
+              .nullable(false)
+              .build();
         };
 
     Optional<Expression> exprA = converterA.convert(concatCall, topLevelConverter);
@@ -125,4 +127,65 @@ void testMergeOrderDeterminesFunctionPrecedence() {
         funcB.declaration().getAnchor().urn(),
         "converterB should use last concat function (from collection1)");
   }
+
+  @Test
+  void testLtrimMergeOrderWithDefaultExtensions() {
+    // This test verifies precedence between a custom ltrim (from collection2 with
+    // extension:com.domain:string) and the default extension catalog's ltrim
+    // (extension:io.substrait:functions_string).
+    // The FunctionConverter uses a "last-wins" strategy.
+
+    // Merge default extensions with collection2 - collection2's ltrim should be last
+    SimpleExtension.ExtensionCollection defaultWithCustom = extensions.merge(collection2);
+
+    // Merge collection2 with default extensions - default ltrim should be last
+    SimpleExtension.ExtensionCollection customWithDefault = collection2.merge(extensions);
+
+    ScalarFunctionConverter converterA =
+        new ScalarFunctionConverter(defaultWithCustom.scalarFunctions(), typeFactory);
+    ScalarFunctionConverter converterB =
+        new ScalarFunctionConverter(customWithDefault.scalarFunctions(), typeFactory);
+
+    // Create a TRIM(LEADING ' ' FROM 'test') call which uses TrimFunctionMapper to map to ltrim
+    RexBuilder rexBuilder = new RexBuilder(typeFactory);
+    RexCall trimCall =
+        (RexCall)
+            rexBuilder.makeCall(
+                SqlStdOperatorTable.TRIM,
+                rexBuilder.makeFlag(Flag.LEADING),
+                rexBuilder.makeLiteral(" "),
+                rexBuilder.makeLiteral("test"));
+
+    java.util.function.Function<RexNode, Expression> topLevelConverter =
+        rexNode -> {
+          org.apache.calcite.rex.RexLiteral lit = (org.apache.calcite.rex.RexLiteral) rexNode;
+          Object value = lit.getValue();
+          if (value == null) {
+            return Expression.StrLiteral.builder().value("").nullable(true).build();
+          }
+          // Convert any literal value to string
+          return Expression.StrLiteral.builder().value(value.toString()).nullable(false).build();
+        };
+
+    Optional<Expression> exprA = converterA.convert(trimCall, topLevelConverter);
+    Optional<Expression> exprB = converterB.convert(trimCall, topLevelConverter);
+
+    // Both should successfully convert
+    assertNotNull(exprA);
+    assertNotNull(exprB);
+
+    Expression.ScalarFunctionInvocation funcA = (Expression.ScalarFunctionInvocation) exprA.get();
+    // converterA should use collection2's custom ltrim (last)
+    assertEquals(
+        "extension:com.domain:string",
+        funcA.declaration().getAnchor().urn(),
+        "converterA should use last ltrim (custom from collection2)");
+
+    Expression.ScalarFunctionInvocation funcB = (Expression.ScalarFunctionInvocation) exprB.get();
+    // converterB should use default extensions' ltrim (last)
+    assertEquals(
+        "extension:io.substrait:functions_string",
+        funcB.declaration().getAnchor().urn(),
+        "converterB should use last ltrim (from default extensions)");
+  }
 }

isthmus/src/test/resources/extensions/functions_duplicate_urn1.yaml

@@ -3,15 +3,6 @@
 urn: extension:io.substrait:functions_string
 
 scalar_functions:
-  - name: "ltrim"
-    description: "left trim from standard functions"
-    impls:
-      - args:
-          - name: str
-            value: string
-          - name: chars
-            value: string
-        return: string
   - name: "concat"
     description: "concatenate strings"
     impls: