From e0964741b16e53d1ce5932295d1410ce3ab6f9fe Mon Sep 17 00:00:00 2001 From: "Mark S. Lewis" Date: Wed, 17 Apr 2024 17:58:01 +0100 Subject: [PATCH] feat: allow deployment time selection of logging framework 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 --- build.gradle.kts | 3 ++- core/build.gradle.kts | 3 ++- .../io/substrait/extension/SimpleExtension.java | 2 +- core/src/main/java/io/substrait/type/YamlRead.java | 2 +- gradle.properties | 2 +- isthmus/build.gradle.kts | 3 +++ .../expression/FieldSelectionConverter.java | 14 ++++++++------ .../isthmus/expression/FunctionConverter.java | 2 +- readme.md | 9 +++++++++ 9 files changed, 28 insertions(+), 12 deletions(-) diff --git a/build.gradle.kts b/build.gradle.kts index 0440bf374..2cefa412f 100644 --- a/build.gradle.kts +++ b/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") diff --git a/core/build.gradle.kts b/core/build.gradle.kts index 43ce972a4..c0e049ae3 100644 --- a/core/build.gradle.kts +++ b/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") diff --git a/core/src/main/java/io/substrait/extension/SimpleExtension.java b/core/src/main/java/io/substrait/extension/SimpleExtension.java index 3bbed56c8..29ba5fe2c 100644 --- a/core/src/main/java/io/substrait/extension/SimpleExtension.java +++ b/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(), diff --git a/core/src/main/java/io/substrait/type/YamlRead.java b/core/src/main/java/io/substrait/type/YamlRead.java index 9d3f1230d..89f8a9163 100644 --- a/core/src/main/java/io/substrait/type/YamlRead.java +++ b/core/src/main/java/io/substrait/type/YamlRead.java @@ -56,7 +56,7 @@ private static Stream 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), diff --git a/gradle.properties b/gradle.properties index 36cea6f3e..01ec4bb57 100644 --- a/gradle.properties +++ b/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.29.1 diff --git a/isthmus/build.gradle.kts b/isthmus/build.gradle.kts index ab902f481..7ca1bfc8a 100644 --- a/isthmus/build.gradle.kts +++ b/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") diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java index c5e93b5a9..ab6233f54 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/expression/FieldSelectionConverter.java @@ -32,10 +32,12 @@ public Optional 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 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 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(); } diff --git a/isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java b/isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java index ebe6336f5..8d6ae849c 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/expression/FunctionConverter.java +++ b/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; } diff --git a/readme.md b/readme.md index 31c33d348..19947f92c 100644 --- a/readme.md +++ b/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/)