Copilot commented on code in PR #12699: URL: https://github.com/apache/gravitino/pull/12699#discussion_r3877372866
########## trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/util/json/TestJsonCodec.java: ########## @@ -0,0 +1,105 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.gravitino.trino.connector.util.json; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import io.trino.FeaturesConfig; +import java.util.Set; +import org.junit.jupiter.api.Test; + +class TestJsonCodec { + + static class NoArgManager {} Review Comment: `NoArgManager` has only an implicit package-private no-arg constructor, but `JsonCodec.instantiateBlockEncodingManager(...)` uses `getConstructor()` (public-only). This test will fail with `NoSuchMethodException`; make the no-arg constructor explicitly public so it exercises the intended code path. ########## trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/util/json/JsonCodec.java: ########## @@ -103,14 +111,123 @@ static TypeManager createTypeManager(ClassLoader classLoader) { static BlockEncodingSerde createBlockEncodingSerde(TypeManager typeManager) throws Exception { ClassLoader classLoader = typeManager.getClass().getClassLoader(); - Class blockEncodingManagerClass = + Class<?> blockEncodingManagerClass = classLoader.loadClass("io.trino.metadata.BlockEncodingManager"); - Class internalBlockEncodingSerdeClass = + Class<?> internalBlockEncodingSerdeClass = classLoader.loadClass("io.trino.metadata.InternalBlockEncodingSerde"); + Object blockEncodingManager = + instantiateBlockEncodingManager(blockEncodingManagerClass, classLoader); return (BlockEncodingSerde) internalBlockEncodingSerdeClass .getConstructor(blockEncodingManagerClass, TypeManager.class) - .newInstance(blockEncodingManagerClass.getConstructor().newInstance(), typeManager); + .newInstance(blockEncodingManager, typeManager); + } + + /** + * Instantiate BlockEncodingManager across Trino/Starburst variants. OSS Trino exposes a public + * no-arg constructor; Starburst replaces it with BlockEncodingManager(FeaturesConfig) (used to + * gate type-specific encodings via feature flags). Newer Trino branches additionally publish a + * {@code Set<BlockEncoding>} variant for Guice multibindings. We probe each known signature in + * order, then fall back to a generic constructor scan that fills unknown reference parameters + * with default values as a last-resort compatibility mechanism. + */ + @VisibleForTesting + static Object instantiateBlockEncodingManager( + Class<?> blockEncodingManagerClass, ClassLoader classLoader) throws Exception { + try { + Object instance = blockEncodingManagerClass.getConstructor().newInstance(); + LOG.debug("Instantiated BlockEncodingManager with its public no-argument constructor"); + return instance; + } catch (NoSuchMethodException ignored) { + // fall through to parameterized variants + } + + try { + Class<?> featuresConfigClass = classLoader.loadClass("io.trino.FeaturesConfig"); + Constructor<?> ctor = blockEncodingManagerClass.getConstructor(featuresConfigClass); + Object featuresConfig = featuresConfigClass.getConstructor().newInstance(); + Object instance = ctor.newInstance(featuresConfig); + LOG.debug("Instantiated BlockEncodingManager with FeaturesConfig"); + return instance; + } catch (NoSuchMethodException | ClassNotFoundException ignored) { + // fall through + } + + try { + Constructor<?> setCtor = blockEncodingManagerClass.getConstructor(Set.class); + Object instance = setCtor.newInstance(Collections.emptySet()); + LOG.debug("Instantiated BlockEncodingManager with an empty BlockEncoding set"); + return instance; + } catch (NoSuchMethodException ignored) { + // fall through to last-resort scan + } Review Comment: `instantiateBlockEncodingManager` only falls through on `NoSuchMethodException`/`ClassNotFoundException`. If a known constructor exists but fails to invoke due to reflection/module access (e.g., `IllegalAccessException`, `InaccessibleObjectException`) or constructor-side failures, the method throws immediately and skips the later compatibility scan. Treat reflective invocation failures as "try next" to preserve the intended cross-version fallback behavior (and log at debug). ########## trino-connector/trino-connector/src/test/java/org/apache/gravitino/trino/connector/util/json/TestJsonCodec.java: ########## @@ -0,0 +1,105 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.gravitino.trino.connector.util.json; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import io.trino.FeaturesConfig; +import java.util.Set; +import org.junit.jupiter.api.Test; + +class TestJsonCodec { + + static class NoArgManager {} + + static class FeaturesConfigManager { + final FeaturesConfig featuresConfig; + + public FeaturesConfigManager(FeaturesConfig featuresConfig) { + this.featuresConfig = featuresConfig; + } + } + + static class SetManager { + final Set<?> encodings; + + public SetManager(Set<?> encodings) { + this.encodings = encodings; + } + } + + static class FallbackManager { + final int flag; + + private FallbackManager(int flag) { + this.flag = flag; + } + } + + static class UnresolvableDependency { + UnresolvableDependency(String required) {} + } + + static class UnresolvableManager { + final UnresolvableDependency dependency; + + private UnresolvableManager(UnresolvableDependency dependency) { + this.dependency = dependency; + } + } + + private static final ClassLoader CLASS_LOADER = TestJsonCodec.class.getClassLoader(); + + @Test + void testPrefersNoArgConstructor() throws Exception { + Object instance = JsonCodec.instantiateBlockEncodingManager(NoArgManager.class, CLASS_LOADER); + assertThat(instance).isInstanceOf(NoArgManager.class); + } + + @Test + void testFallsBackToFeaturesConfigConstructor() throws Exception { + Object instance = + JsonCodec.instantiateBlockEncodingManager(FeaturesConfigManager.class, CLASS_LOADER); + assertThat(instance).isInstanceOfSatisfying(FeaturesConfigManager.class, m -> {}); + } Review Comment: `testFallsBackToFeaturesConfigConstructor` currently asserts nothing about the constructed instance, so it won’t detect regressions (e.g., if fallback picks a different constructor). Assert that the injected `FeaturesConfig` is non-null to verify the intended constructor was used. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
