feat: allow deployment time selection of logging framework #243 (#244)

Depend on slf4j-api instead of the slf4j-jdk14 logging framework. This allows
consumers to select their preferred logging framework at deployment time by
adding an appropriate SLF4J provider to their runtime classpath.

If no SLF4J provider is available at runtime, no logging output will be
produced. The code still functions as expected.

The isthmus CLI command continues to log using java.util.logging by including
the slf4j-jdk14 provider in its runtime classpath.

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>

build.gradle.kts

@@ -13,6 +13,7 @@ plugins {
 
 var IMMUTABLES_VERSION = properties.get("immutables.version")
 var JUNIT_VERSION = properties.get("junit.version")
+var SLF4J_VERSION = properties.get("slf4j.version")
 
 repositories { mavenCentral() }
 
@@ -21,7 +22,7 @@ java { toolchain { languageVersion.set(JavaLanguageVersion.of(17)) } }
 dependencies {
   testImplementation("org.junit.jupiter:junit-jupiter-api:${JUNIT_VERSION}")
   testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:${JUNIT_VERSION}")
-  implementation("org.slf4j:slf4j-jdk14:1.7.30")
+  implementation("org.slf4j:slf4j-api:${SLF4J_VERSION}")
   annotationProcessor("org.immutables:value:${IMMUTABLES_VERSION}")
   compileOnly("org.immutables:value-annotations:${IMMUTABLES_VERSION}")
   annotationProcessor("com.github.bsideup.jabel:jabel-javac-plugin:0.4.2")

core/build.gradle.kts

@@ -71,6 +71,7 @@ val ANTLR_VERSION = properties.get("antlr.version")
 var IMMUTABLES_VERSION = properties.get("immutables.version")
 var JACKSON_VERSION = properties.get("jackson.version")
 var JUNIT_VERSION = properties.get("junit.version")
+var SLF4J_VERSION = properties.get("slf4j.version")
 
 dependencies {
   testImplementation("org.junit.jupiter:junit-jupiter-api:${JUNIT_VERSION}")
@@ -85,7 +86,7 @@ dependencies {
 
   antlr("org.antlr:antlr4:${ANTLR_VERSION}")
   implementation("org.antlr:antlr4-runtime:${ANTLR_VERSION}")
-  implementation("org.slf4j:slf4j-jdk14:1.7.30")
+  implementation("org.slf4j:slf4j-api:${SLF4J_VERSION}")
   annotationProcessor("org.immutables:value:${IMMUTABLES_VERSION}")
   compileOnly("org.immutables:value-annotations:${IMMUTABLES_VERSION}")
   annotationProcessor("com.github.bsideup.jabel:jabel-javac-plugin:0.4.2")

core/src/main/java/io/substrait/extension/SimpleExtension.java

@@ -796,7 +796,7 @@ public static ExtensionCollection buildExtensionCollection(
             .windowFunctions(allWindowFunctionVariants)
             .addAllTypes(extensionSignatures.types())
             .build();
-    logger.debug(
+    logger.atDebug().log(
         "Loaded {} aggregate functions and {} scalar functions from {}.",
         collection.aggregateFunctions().size(),
         collection.scalarFunctions().size(),

core/src/main/java/io/substrait/type/YamlRead.java

@@ -56,7 +56,7 @@ private static Stream<SimpleExtension.Function> parse(String name) {
               .registerModule(Deserializers.MODULE);
       var doc = mapper.readValue(new File(name), SimpleExtension.ExtensionSignatures.class);
 
-      logger.debug(
+      logger.atDebug().log(
           "Parsed {} functions in file {}.",
           Optional.ofNullable(doc.scalars()).map(List::size).orElse(0)
               + Optional.ofNullable(doc.aggregates()).map(List::size).orElse(0),

gradle.properties

@@ -21,7 +21,7 @@ immutables.version=2.10.1
 jackson.version=2.16.1
 junit.version=5.8.1
 protobuf.version=3.17.1
-slf4j.version=1.7.25
+slf4j.version=2.0.13
 
 #version that is going to be updated automatically by releases
 version = 0.30.0

isthmus/build.gradle.kts

@@ -76,6 +76,7 @@ var GUAVA_VERSION = properties.get("guava.version")
 var IMMUTABLES_VERSION = properties.get("immutables.version")
 var JACKSON_VERSION = properties.get("jackson.version")
 var JUNIT_VERSION = properties.get("junit.version")
+var SLF4J_VERSION = properties.get("slf4j.version")
 
 dependencies {
   implementation(project(":core"))
@@ -97,6 +98,8 @@ dependencies {
   implementation("com.google.code.findbugs:jsr305:3.0.2")
   implementation("com.github.ben-manes.caffeine:caffeine:3.0.4")
   implementation("org.immutables:value-annotations:${IMMUTABLES_VERSION}")
+  implementation("org.slf4j:slf4j-api:${SLF4J_VERSION}")
+  runtimeOnly("org.slf4j:slf4j-jdk14:${SLF4J_VERSION}")
   annotationProcessor("org.immutables:value:${IMMUTABLES_VERSION}")
   testImplementation("org.apache.calcite:calcite-plus:${CALCITE_VERSION}")
   annotationProcessor("com.github.bsideup.jabel:jabel-javac-plugin:0.4.2")

isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java

@@ -32,10 +32,12 @@ public Optional<Expression> convert(
     var reference = call.getOperands().get(1);
 
     if (reference.getKind() != SqlKind.LITERAL || !(reference instanceof RexLiteral)) {
-      logger.warn(
-          "Found item operator without literal kind/type. This isn't handled well. Reference was {} with toString {}.",
-          reference.getKind().name(),
-          reference);
+      logger
+          .atWarn()
+          .log(
+              "Found item operator without literal kind/type. This isn't handled well. Reference was {} with toString {}.",
+              reference.getKind().name(),
+              reference);
       return Optional.empty();
     }
 
@@ -99,13 +101,13 @@ private Optional<Integer> toInt(Expression.Literal l) {
     } else if (l instanceof Expression.I64Literal i64) {
       return Optional.of((int) i64.value());
     }
-    logger.warn("Literal expected to be int type but was not. {}.", l);
+    logger.atWarn().log("Literal expected to be int type but was not. {}.", l);
     return Optional.empty();
   }
 
   public Optional<String> toString(Expression.Literal l) {
     if (!(l instanceof Expression.FixedCharLiteral)) {
-      logger.warn("Literal expected to be char type but was not. {}", l);
+      logger.atWarn().log("Literal expected to be char type but was not. {}", l);
       return Optional.empty();
     }
 

isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java

@@ -81,7 +81,7 @@ public FunctionConverter(
     for (String key : alm.keySet()) {
       var sigs = calciteOperators.get(key);
       if (sigs == null) {
-        logger.info("Dropping function due to no binding: {}", key);
+        logger.atInfo().log("Dropping function due to no binding: {}", key);
         continue;
       }
 

readme.md

@@ -23,5 +23,14 @@ Another way to get an idea of what Substrait plans look like is to use our scrip
 ./isthmus/src/test/script/tpch_smoke.sh
 ```
 
+## Logging
+This project uses the [SLF4J](https://www.slf4j.org/) logging API. If you are using the Substrait Java core component as a dependency in your own project, you should consider including an appropriate [SLF4J logging provider](https://www.slf4j.org/manual.html#swapping) in your runtime classpath. If you do not include a logging provider in your classpath, the code will still work correctly but you will not receive any logging output and might see the following warning in your standard error output:
+
+```
+SLF4J(W): No SLF4J providers were found.
+SLF4J(W): Defaulting to no-operation (NOP) logger implementation
+SLF4J(W): See https://www.slf4j.org/codes.html#noProviders for further details.
+```
+
 ## Getting Involved
 To learn more, head over [Substrait](https://substrait.io/), our parent project and join our [community](https://substrait.io/community/)