Verify JVM configuration in Snowflake connector

docs/src/main/sphinx/connector/snowflake.md

@@ -33,15 +33,9 @@ snowflake.role=role
 snowflake.warehouse=warehouse
 ```
 
-### Arrow serialization support
-
-This is an experimental feature which introduces support for using Apache Arrow
-as the serialization format when reading from Snowflake.  Please note there are
-a few caveats:
-
-- Using Apache Arrow serialization is disabled by default. In order to enable
-  it,  add `--add-opens=java.base/java.nio=ALL-UNNAMED` to the Trino
-  {ref}`jvm-config`.
+Snowflake connector uses Apache Arrow as the serialization format when
+reading from Snowflake which requires additional JVM arguments. Add
+`--add-opens=java.base/java.nio=ALL-UNNAMED` to the Trino {ref}`jvm-config`.
 
 ### Multiple Snowflake databases or accounts
 

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/JdkCompatibilityChecks.java

@@ -0,0 +1,78 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.plugin.base;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Multimap;
+import com.google.inject.Binder;
+
+import java.lang.management.ManagementFactory;
+import java.util.List;
+import java.util.regex.Pattern;
+
+import static java.lang.String.format;
+import static java.util.Objects.requireNonNull;
+
+public class JdkCompatibilityChecks
+{
+    private final String inputArguments;
+
+    private static final JdkCompatibilityChecks INSTANCE = new JdkCompatibilityChecks(
+            ManagementFactory.getRuntimeMXBean().getInputArguments());
+
+    @VisibleForTesting
+    JdkCompatibilityChecks(List<String> inputArguments)
+    {
+        this.inputArguments = Joiner.on(" ")
+                .skipNulls()
+                .join(requireNonNull(inputArguments, "inputArguments is null"));
+    }
+
+    public static void verifyConnectorAccessOpened(Binder binder, String connectorName, Multimap<String, String> modules)
+    {
+        INSTANCE.verifyAccessOpened(wrap(binder), format("Connector '%s'", connectorName), modules);
+    }
+
+    @VisibleForTesting
+    void verifyAccessOpened(ThrowableSettable throwableSettable, String description, Multimap<String, String> modules)
+    {
+        ImmutableList.Builder<String> missingJvmArguments = ImmutableList.builder();
+        for (String fromModule : modules.keySet()) {
+            for (String toModule : modules.get(fromModule)) {
+                String requiredJvmArgument = format(".*?--add-opens[\\s=]%s/%s=ALL-UNNAMED.*?", Pattern.quote(fromModule), Pattern.quote(toModule));
+                if (!inputArguments.matches(requiredJvmArgument)) {
+                    missingJvmArguments.add(format("--add-opens=%s/%s=ALL-UNNAMED", fromModule, toModule));
+                }
+            }
+        }
+
+        List<String> requiredJvmArguments = missingJvmArguments.build();
+        if (!requiredJvmArguments.isEmpty()) {
+            throwableSettable.setThrowable(new IllegalStateException(format("%s requires additional JVM argument(s). Please add the following to the JVM configuration: '%s'", description, String.join(" ", requiredJvmArguments))));
+        }
+    }
+
+    private static ThrowableSettable wrap(Binder binder)
+    {
+        return throwable -> binder.addError(throwable.getMessage());
+    }
+
+    @VisibleForTesting
+    interface ThrowableSettable
+    {
+        void setThrowable(Throwable throwable);
+    }
+}

lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/TestJdkCompatibilityChecks.java

@@ -0,0 +1,105 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.plugin.base;
+
+import com.google.common.collect.ImmutableMultimap;
+import com.google.inject.Binder;
+import com.google.inject.CreationException;
+import com.google.inject.Guice;
+import com.google.inject.Module;
+import org.junit.jupiter.api.Test;
+
+import java.util.List;
+
+import static io.trino.plugin.base.JdkCompatibilityChecks.verifyConnectorAccessOpened;
+import static java.util.Objects.requireNonNull;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+public class TestJdkCompatibilityChecks
+{
+    @Test
+    public void shouldThrowWhenAccessIsNotGranted()
+    {
+        ThrowableSettableMock errorSink = new ThrowableSettableMock();
+        new JdkCompatibilityChecks(List.of()).verifyAccessOpened(errorSink, "Connector 'snowflake'", ImmutableMultimap.of(
+                "java.base", "java.nio"));
+
+        assertThat(errorSink.getThrowable())
+                .isInstanceOf(IllegalStateException.class)
+                .hasMessage("Connector 'snowflake' requires additional JVM argument(s). Please add the following to the JVM configuration: '--add-opens=java.base/java.nio=ALL-UNNAMED'");
+    }
+
+    @Test
+    public void shouldThrowWhenOneOfAccessGrantsIsMissing()
+    {
+        ThrowableSettableMock errorSink = new ThrowableSettableMock();
+        new JdkCompatibilityChecks(List.of("--add-opens", "java.base/java.nio=ALL-UNNAMED")).verifyAccessOpened(errorSink, "Connector 'snowflake'", ImmutableMultimap.of(
+                "java.base", "java.nio",
+                "java.base", "java.lang"));
+
+        assertThat(errorSink.getThrowable())
+                .isInstanceOf(IllegalStateException.class)
+                .hasMessage("Connector 'snowflake' requires additional JVM argument(s). Please add the following to the JVM configuration: '--add-opens=java.base/java.lang=ALL-UNNAMED'");
+    }
+
+    @Test
+    public void shouldSetErrorOnBinder()
+    {
+        assertThatThrownBy(() -> Guice.createInjector(new TestingModule()))
+                .isInstanceOf(CreationException.class)
+                .hasMessageContaining("Unable to create injector, see the following errors:")
+                .hasMessageContaining("1) Connector 'Testing' requires additional JVM argument(s). Please add the following to the JVM configuration: '--add-opens=java.base/java.non.existing=ALL-UNNAMED --add-opens=java.base/java.this.should.fail=ALL-UNNAMED'");
+    }
+
+    @Test
+    public void shouldNotThrowWhenAllAccessGrantsArePresent()
+    {
+        ThrowableSettableMock errorSink = new ThrowableSettableMock();
+        new JdkCompatibilityChecks(List.of("--add-opens=java.base/java.nio=ALL-UNNAMED", "--add-opens java.base/java.lang=ALL-UNNAMED")).verifyAccessOpened(errorSink, "Connector 'snowflake'", ImmutableMultimap.of(
+                "java.base", "java.nio",
+                "java.base", "java.lang"));
+
+        assertThat(errorSink.getThrowable()).isNull();
+    }
+
+    private static class TestingModule
+            implements Module
+    {
+        @Override
+        public void configure(Binder binder)
+        {
+            verifyConnectorAccessOpened(binder, "Testing", ImmutableMultimap.of(
+                    "java.base", "java.non.existing",
+                    "java.base", "java.this.should.fail"));
+        }
+    }
+
+    private static class ThrowableSettableMock
+            implements JdkCompatibilityChecks.ThrowableSettable
+    {
+        private Throwable throwable;
+
+        @Override
+        public void setThrowable(Throwable throwable)
+        {
+            this.throwable = requireNonNull(throwable, "throwable is null");
+        }
+
+        public Throwable getThrowable()
+        {
+            return throwable;
+        }
+    }
+}

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryConnectorModule.java

@@ -15,6 +15,7 @@
 
 import com.google.api.gax.rpc.FixedHeaderProvider;
 import com.google.api.gax.rpc.HeaderProvider;
+import com.google.common.collect.ImmutableMultimap;
 import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.inject.Binder;
 import com.google.inject.Key;
@@ -24,6 +25,7 @@
 import com.google.inject.multibindings.Multibinder;
 import com.google.inject.multibindings.OptionalBinder;
 import io.airlift.configuration.AbstractConfigurationAwareModule;
+import io.trino.plugin.base.JdkCompatibilityChecks;
 import io.trino.plugin.base.logging.FormatInterpolator;
 import io.trino.plugin.base.logging.SessionInterpolatedValues;
 import io.trino.plugin.base.session.SessionPropertiesProvider;
@@ -34,11 +36,7 @@
 import io.trino.spi.function.table.ConnectorTableFunction;
 import io.trino.spi.procedure.Procedure;
 
-import java.lang.management.ManagementFactory;
-import java.util.Set;
 import java.util.concurrent.ExecutorService;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 
 import static com.google.common.util.concurrent.MoreExecutors.listeningDecorator;
 import static com.google.inject.multibindings.Multibinder.newSetBinder;
@@ -47,10 +45,8 @@
 import static io.airlift.configuration.ConditionalModule.conditionalModule;
 import static io.airlift.configuration.ConfigBinder.configBinder;
 import static io.trino.plugin.base.ClosingBinder.closingBinder;
-import static io.trino.plugin.bigquery.BigQueryConfig.ARROW_SERIALIZATION_ENABLED;
 import static java.util.concurrent.Executors.newCachedThreadPool;
 import static java.util.concurrent.Executors.newFixedThreadPool;
-import static java.util.stream.Collectors.toSet;
 
 public class BigQueryConnectorModule
         extends AbstractConfigurationAwareModule
@@ -87,7 +83,10 @@ protected void setup(Binder binder)
             install(conditionalModule(
                     BigQueryConfig.class,
                     BigQueryConfig::isArrowSerializationEnabled,
-                    ClientModule::verifyPackageAccessAllowed));
+                    innerBinder -> JdkCompatibilityChecks.verifyConnectorAccessOpened(
+                            innerBinder,
+                            "bigquery",
+                            ImmutableMultimap.of("java.base", "java.nio"))));
             newSetBinder(binder, ConnectorTableFunction.class).addBinding().toProvider(Query.class).in(Scopes.SINGLETON);
             newSetBinder(binder, Procedure.class).addBinding().toProvider(ExecuteProcedure.class).in(Scopes.SINGLETON);
             newSetBinder(binder, SessionPropertiesProvider.class).addBinding().to(BigQuerySessionProperties.class).in(Scopes.SINGLETON);
@@ -141,34 +140,6 @@ public ExecutorService provideExecutor(CatalogName catalogName)
         {
             return newCachedThreadPool(daemonThreadsNamed("bigquery-" + catalogName + "-%s"));
         }
-
-        /**
-         * Apache Arrow requires reflective access to certain Java internals prohibited since Java 17.
-         * Adds an error to the {@code binder} if required --add-opens is not passed to the JVM.
-         */
-        private static void verifyPackageAccessAllowed(Binder binder)
-        {
-            // Match an --add-opens argument that opens a package to unnamed modules.
-            // The first group is the opened package.
-            Pattern argPattern = Pattern.compile(
-                    "^--add-opens=(.*)=([A-Za-z0-9_.]+,)*ALL-UNNAMED(,[A-Za-z0-9_.]+)*$");
-            // We don't need to check for values in separate arguments because
-            // they are joined with "=" before we get them.
-
-            Set<String> openedModules = ManagementFactory.getRuntimeMXBean()
-                    .getInputArguments()
-                    .stream()
-                    .map(argPattern::matcher)
-                    .filter(Matcher::matches)
-                    .map(matcher -> matcher.group(1))
-                    .collect(toSet());
-
-            if (!openedModules.contains("java.base/java.nio")) {
-                binder.addError(
-                        "BigQuery connector requires additional JVM arguments to run when '" + ARROW_SERIALIZATION_ENABLED + "' is enabled. " +
-                                "Please add '--add-opens=java.base/java.nio=ALL-UNNAMED' to the JVM configuration.");
-            }
-        }
     }
 
     public static class StaticCredentialsModule

plugin/trino-snowflake/src/main/java/io/trino/plugin/snowflake/SnowflakeClientModule.java

@@ -13,12 +13,14 @@
  */
 package io.trino.plugin.snowflake;
 
+import com.google.common.collect.ImmutableMultimap;
 import com.google.inject.Binder;
 import com.google.inject.Module;
 import com.google.inject.Provides;
 import com.google.inject.Scopes;
 import com.google.inject.Singleton;
 import io.opentelemetry.api.OpenTelemetry;
+import io.trino.plugin.base.JdkCompatibilityChecks;
 import io.trino.plugin.jdbc.BaseJdbcConfig;
 import io.trino.plugin.jdbc.ConnectionFactory;
 import io.trino.plugin.jdbc.DriverConnectionFactory;
@@ -46,6 +48,11 @@ public class SnowflakeClientModule
     @Override
     public void configure(Binder binder)
     {
+        // Check reflective access allowed - required by Apache Arrow usage in Snowflake JDBC driver
+        JdkCompatibilityChecks.verifyConnectorAccessOpened(
+                binder,
+                "snowflake",
+                ImmutableMultimap.of("java.base", "java.nio"));
         binder.bind(JdbcClient.class).annotatedWith(ForBaseJdbc.class).to(SnowflakeClient.class).in(Scopes.SINGLETON);
         configBinder(binder).bindConfig(SnowflakeConfig.class);
         configBinder(binder).bindConfig(TypeHandlingJdbcConfig.class);