diqiu50 commented on PR #13142: URL: https://github.com/apache/gravitino/pull/13142#issuecomment-5666762785
> Reviewed `c8dc47a826`. Building the complete routine specification in the connector is the right layer for this fix, and enabling the JUnit ITs is useful. I found three remaining issues in the conversion. > > **1. Valid routine bodies can receive an extra `RETURN`.** > > [`startsWithKeyword`](https://github.com/apache/gravitino/blob/c8dc47a8269c2fee106b8b3871bd4fad52dc702b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoMetadata.java#L1053) requires whitespace after the keyword and does not skip leading comments. Consequently: > > * `/* comment */ RETURN x + 1` becomes `RETURN /* comment */ RETURN x + 1`. > * `RETURN/* comment */ x + 1` becomes `RETURN RETURN/* comment */ x + 1`. > * `RETURN(x + 1)` becomes `RETURN RETURN(x + 1)`. > > I checked these generated specifications with the real Trino 440 parser. The first two fail parsing, while the third parses as a call to a function named `RETURN`, changing the meaning of the body. This can reintroduce schema-wide listing failures or invocation failures for otherwise valid input. Please recognize SQL tokens/control statements rather than whitespace-delimited prefixes, and cover comments and punctuation boundaries in tests. > > **2. `Type.getDisplayName()` is not safe SQL serialization for nested field names.** > > The [parameter/return type generation](https://github.com/apache/gravitino/blob/c8dc47a8269c2fee106b8b3871bd4fad52dc702b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoMetadata.java#L1025) quotes parameter names, but not names inside row types. For example, a struct field named `value-with-dash` needs `row("value-with-dash" integer)`. Trino 440's [`RowType.getDisplayName()`](https://github.com/trinodb/trino/blob/440/core/trino-spi/src/main/java/io/trino/spi/type/RowType.java#L219) emits the field name without quoting, producing `row(value-with-dash integer)`, which the parser rejects. > > Please serialize types with proper identifier escaping, including nested rows in arrays/maps and return types. The current `TrinoException` catch does not protect against these parse errors: construction succeeds and parsing happens later inside Trino. > > **3. Full specifications bypass the `SECURITY INVOKER` fix.** > > The [early return for `FUNCTION ...`](https://github.com/apache/gravitino/blob/c8dc47a8269c2fee106b8b3871bd4fad52dc702b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoMetadata.java#L1014) preserves declarations without a security characteristic. The existing pass-through test itself uses: > > ```sql > FUNCTION my_func(x integer) RETURNS bigint BEGIN RETURN x; END > ``` > > This still defaults to DEFINER, while the constructed `LanguageFunction` has no owner. Without a separately available definer identity, invocation fails with `No identity for SECURITY DEFINER function`. The latest commit fixes the generated-declaration path but leaves this supported compatibility path unresolved. > > Please define the full-specification policy explicitly: normalize an omitted security characteristic to INVOKER if that is the intended contract, and reject unsupported explicit DEFINER declarations rather than silently changing their semantics. Also validate that the full declaration agrees with the registered name, parameters, and return type. This path needs an invocation test, not only a pass-through string assertion. > > For implementation, a dedicated routine converter would keep SQL formatting and compatibility rules out of `GravitinoMetadata`. Using Trino's parser/AST/formatter is worth considering, subject to the connector's multi-version dependency constraints. At minimum, test the generated SQL with the actual parser and add invocation coverage where parsing alone is insufficient. > > The earlier parameter-name, aggregate-filtering, and UI wording comments are addressed in this revision. Root `build.gradle.kts` also supplies the test environment variables, so I do not see the previously reported missing-environment issue. > > Validation for this review: source inspection and standalone Trino 440 parser probes; I did not rerun the full Docker integration suite. Fixed -- 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]
