Copilot commented on code in PR #13142:
URL: https://github.com/apache/gravitino/pull/13142#discussion_r4003652990
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoMetadata.java:
##########
@@ -880,27 +880,72 @@ public Collection<LanguageFunction> getLanguageFunctions(
* Converts a Gravitino function to a collection of Trino LanguageFunction
instances. Only SQL
* implementations with TRINO runtime are included. Each definition with a
Trino SQL
* implementation produces one LanguageFunction. The signature token is
generated from the
- * function name and parameter types.
+ * function name and parameter types, and the stored SQL body is expanded
into a complete Trino
+ * function specification.
*/
private Collection<LanguageFunction> toLanguageFunctions(Function function) {
List<LanguageFunction> result = new ArrayList<>();
for (FunctionDefinition definition : function.definitions()) {
+ if (definition.returnType() == null) {
+ LOG.warn("Skipping function %s: definition has no return type",
function.name());
+ continue;
+ }
for (FunctionImpl impl : definition.impls()) {
if (!isTrinoSqlImplementation(impl)) {
continue;
}
String sql = ((SQLImpl) impl).sql();
try {
String signatureToken = buildSignatureToken(function.name(),
definition.parameters());
- result.add(new LanguageFunction(signatureToken, sql, List.of(),
Optional.empty()));
+ String specification = buildFunctionSpecification(function,
definition, sql);
+ result.add(
+ new LanguageFunction(signatureToken, specification, List.of(),
Optional.empty()));
} catch (TrinoException e) {
- LOG.warn(e, "Failed to build signature token for function %s",
function.name());
+ LOG.warn(e, "Failed to build language function for %s",
function.name());
}
}
}
return result;
}
+ /**
+ * Builds the SQL routine specification Trino expects for a language
function: {@code FUNCTION
+ * name(params) RETURNS type [NOT] DETERMINISTIC RETURN body}. The stored
body may be a bare
+ * expression, a control statement ({@code RETURN ...} / {@code BEGIN ...
END}), or already a full
+ * specification; only the missing parts are added.
+ */
+ private String buildFunctionSpecification(
+ Function function, FunctionDefinition definition, String sql) {
+ String body = sql.trim();
+ if (startsWithKeyword(body, "FUNCTION")) {
+ return body;
+ }
+ StringBuilder sb = new StringBuilder("FUNCTION
").append(function.name()).append("(");
+ FunctionParam[] params = definition.parameters();
+ for (int i = 0; i < params.length; i++) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ Type trinoType =
metadataAdapter.getDataTypeTransformer().getTrinoType(params[i].dataType());
+ sb.append(params[i].name()).append("
").append(trinoType.getDisplayName());
Review Comment:
`FunctionParam` carries an optional `defaultValue`, but this generated
declaration always emits only the parameter name and type. A function
registered with an optional parameter therefore loses that behavior in Trino,
so calls omitting the parameter can no longer resolve. Preserve the default
expression in the routine specification, or explicitly reject and document
unsupported defaults.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoMetadata.java:
##########
@@ -880,27 +880,72 @@ public Collection<LanguageFunction> getLanguageFunctions(
* Converts a Gravitino function to a collection of Trino LanguageFunction
instances. Only SQL
* implementations with TRINO runtime are included. Each definition with a
Trino SQL
* implementation produces one LanguageFunction. The signature token is
generated from the
- * function name and parameter types.
+ * function name and parameter types, and the stored SQL body is expanded
into a complete Trino
+ * function specification.
*/
private Collection<LanguageFunction> toLanguageFunctions(Function function) {
List<LanguageFunction> result = new ArrayList<>();
for (FunctionDefinition definition : function.definitions()) {
+ if (definition.returnType() == null) {
+ LOG.warn("Skipping function %s: definition has no return type",
function.name());
+ continue;
+ }
for (FunctionImpl impl : definition.impls()) {
if (!isTrinoSqlImplementation(impl)) {
continue;
}
String sql = ((SQLImpl) impl).sql();
try {
String signatureToken = buildSignatureToken(function.name(),
definition.parameters());
- result.add(new LanguageFunction(signatureToken, sql, List.of(),
Optional.empty()));
+ String specification = buildFunctionSpecification(function,
definition, sql);
+ result.add(
+ new LanguageFunction(signatureToken, specification, List.of(),
Optional.empty()));
} catch (TrinoException e) {
- LOG.warn(e, "Failed to build signature token for function %s",
function.name());
+ LOG.warn(e, "Failed to build language function for %s",
function.name());
}
}
}
return result;
}
+ /**
+ * Builds the SQL routine specification Trino expects for a language
function: {@code FUNCTION
+ * name(params) RETURNS type [NOT] DETERMINISTIC RETURN body}. The stored
body may be a bare
+ * expression, a control statement ({@code RETURN ...} / {@code BEGIN ...
END}), or already a full
+ * specification; only the missing parts are added.
+ */
+ private String buildFunctionSpecification(
+ Function function, FunctionDefinition definition, String sql) {
+ String body = sql.trim();
+ if (startsWithKeyword(body, "FUNCTION")) {
+ return body;
+ }
+ StringBuilder sb = new StringBuilder("FUNCTION
").append(function.name()).append("(");
+ FunctionParam[] params = definition.parameters();
+ for (int i = 0; i < params.length; i++) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ Type trinoType =
metadataAdapter.getDataTypeTransformer().getTrinoType(params[i].dataType());
+ sb.append(params[i].name()).append("
").append(trinoType.getDisplayName());
Review Comment:
`FunctionParams` accepts any non-blank parameter name, but this appends the
raw name into Trino's routine signature. A valid Gravitino parameter such as
`select` or `value-with-dash` therefore produces an unparsable specification
and can still make `SHOW FUNCTIONS` fail for the schema. Quote/escape parameter
identifiers using Trino's SQL rules (and keep the signature handling
consistent), or explicitly reject unsupported names before exposing the
function.
##########
.github/workflows/trino-integration-test-action.yml:
##########
@@ -63,9 +63,16 @@ jobs:
# Disable the Trino cascading query integration test, because the
connector jars are private now.
#trino-connector/integration-test/trino-test-tools/run_test.sh
+ # The SQL-file harness above does not execute the JUnit ITs under
+ # trino-connector/integration-test (TrinoUDFIT etc.), so run them
explicitly.
+ - name: Trino JUnit Integration Test
+ id: junitIntegrationTest
+ run: |
+ ./gradlew :trino-connector:integration-test:test
-PskipDockerTests=false -PskipWeb=true
Review Comment:
On a fresh GitHub runner this step starts the JUnit ITs without the
environment variables those tests require. `TrinoITContainers` throws when
`GRAVITINO_ROOT_DIR` is absent, and `BaseIT` reads `GRAVITINO_HOME`; the
preceding shell script exports these only in its own process, so they are not
inherited by this later step. Set the test command's
`GRAVITINO_ROOT_DIR`/`GRAVITINO_HOME` to the workspace (and
`GRAVITINO_TEST=true`) or define them at job scope.
##########
web-v2/web/src/app/catalogs/rightContent/entitiesContent/FunctionDetailsPage.js:
##########
@@ -100,10 +100,19 @@ const buildSignature = (name, definition) => {
)
}
+// Only SQL implementations with the TRINO runtime are exposed as Trino
language functions
+const isTrinoVisible = impl => impl?.language === 'SQL' && impl?.runtime ===
'TRINO'
+
const buildImplDetails = impl => {
const details = [
{ label: 'Language', value: impl?.language || '-' },
- { label: 'Runtime', value: impl?.runtime || '-' }
+ { label: 'Runtime', value: impl?.runtime || '-' },
+ {
+ label: 'Trino Connector',
+ value: isTrinoVisible(impl)
+ ? 'Visible and callable from Trino'
Review Comment:
This label is based only on the routing filter, but the connector also skips
non-scalar definitions, unsupported mapped types, and bodies that fail to
parse. A SQL/TRINO implementation can therefore be shown as “Visible and
callable” while it is absent from Trino. Use eligibility wording or compute the
full exposure result instead of asserting availability.
--
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]
