Copilot commented on code in PR #12527:
URL: https://github.com/apache/gravitino/pull/12527#discussion_r3813796337
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/util/json/JsonCodec.java:
##########
@@ -103,14 +110,107 @@ 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.
+ */
+ private 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
+ }
+
+ NoSuchMethodException lastError = null;
+ for (Constructor<?> ctor :
blockEncodingManagerClass.getDeclaredConstructors()) {
+ try {
+ ctor.setAccessible(true);
+ Class<?>[] paramTypes = ctor.getParameterTypes();
+ Object[] args = new Object[paramTypes.length];
+ for (int i = 0; i < paramTypes.length; i++) {
+ if (Set.class.isAssignableFrom(paramTypes[i])) {
+ args[i] = Collections.emptySet();
+ } else if (paramTypes[i].isPrimitive()) {
+ args[i] = defaultPrimitive(paramTypes[i]);
+ } else {
+ args[i] = tryDefaultInstance(paramTypes[i]);
+ }
+ }
+ Object instance = ctor.newInstance(args);
Review Comment:
The “last-resort” constructor scan may pass `null` for reference parameters
without a no-arg constructor (`tryDefaultInstance` returns null). If a
constructor accepts non-null dependencies, this can either (a) instantiate a
subtly misconfigured `BlockEncodingManager` (hard-to-diagnose behavior
differences) or (b) throw inside the constructor after partial initialization.
Consider tightening the fallback: only attempt constructors where every
non-primitive parameter is a `Set` or has an accessible no-arg constructor;
otherwise skip that constructor and continue.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/util/json/JsonCodec.java:
##########
@@ -103,14 +110,107 @@ 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.
+ */
+ private 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
+ }
+
+ NoSuchMethodException lastError = null;
+ for (Constructor<?> ctor :
blockEncodingManagerClass.getDeclaredConstructors()) {
+ try {
+ ctor.setAccessible(true);
+ Class<?>[] paramTypes = ctor.getParameterTypes();
+ Object[] args = new Object[paramTypes.length];
+ for (int i = 0; i < paramTypes.length; i++) {
+ if (Set.class.isAssignableFrom(paramTypes[i])) {
+ args[i] = Collections.emptySet();
+ } else if (paramTypes[i].isPrimitive()) {
+ args[i] = defaultPrimitive(paramTypes[i]);
+ } else {
+ args[i] = tryDefaultInstance(paramTypes[i]);
+ }
+ }
+ Object instance = ctor.newInstance(args);
Review Comment:
`ctor.setAccessible(true)` can throw `InaccessibleObjectException` (a
runtime exception) on newer JDKs / module boundaries, which is not caught by
the current `catch (ReflectiveOperationException e)` and will abort the
fallback scan. Prefer `ctor.trySetAccessible()` (skip if false), or catch
`RuntimeException` around the accessibility call and continue scanning other
constructors.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorFactory.java:
##########
@@ -192,6 +212,32 @@ private void checkTrinoSpiVersion(ConnectorContext
context, GravitinoConfig conf
}
}
+ @VisibleForTesting
+ static boolean isSecuritySensitivePropertyName(String propertyName) {
+ String normalizedPropertyName =
propertyName.toLowerCase(Locale.ROOT).replaceAll("[._-]", "");
+ return
SECURITY_SENSITIVE_PROPERTY_SUFFIXES.stream().anyMatch(normalizedPropertyName::endsWith);
+ }
+
Review Comment:
`String#replaceAll` uses regex and will compile/execute a pattern on every
call. Since this runs over config keys, consider replacing it with a simple
character filter loop (drop `.`, `_`, `-`) to avoid regex overhead and reduce
allocations.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoDynamicFilter.java:
##########
@@ -55,6 +56,14 @@ public boolean isAwaitable() {
return delegate.isAwaitable();
}
+ // Note: this method is not annotated with @Override because it does not
exist in the
+ // DynamicFilter interface of the baseline open-source Trino SPI version
this connector compiles
+ // against. It is present in newer Trino/Starburst SPI versions, where it is
dispatched at
+ // runtime by signature, providing cross-version compatibility.
+ public OptionalLong getPreferredDynamicFilterTimeout() {
Review Comment:
Always returning `OptionalLong.empty()` ignores any timeout preference
provided by the underlying `delegate` in newer Trino/Starburst SPI
implementations, which can change planning/runtime behavior. To preserve
semantics across versions, consider reflectively invoking
`delegate.getPreferredDynamicFilterTimeout()` when present (same zero-arg
signature) and falling back to `OptionalLong.empty()` only when the method
doesn’t exist or fails.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/util/json/JsonCodec.java:
##########
@@ -103,14 +110,107 @@ 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.
+ */
+ private 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
+ }
Review Comment:
`instantiateBlockEncodingManager` introduces multiple constructor-selection
branches (no-arg, `FeaturesConfig`, `Set`, and reflective fallback) but there
are no unit tests covering these selection rules. Consider adding targeted
tests using small in-test dummy classes (with matching constructor signatures)
to validate selection order and failure behavior.
--
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]
