Conversation
…lity This change updates the UdfParameter.parse method to use the database dialect for determining the correct quote character (backticks for GoogleSQL, double quotes for PostgreSQL). It also refactors the string processing logic to avoid manual index manipulation, leveraging regex-based splitting for the DEFAULT expression. Additionally, it includes a fix for a compilation error in DirectRunnerClient.java by replacing a deprecated/missing stop() call with interrupt(). New unit tests have been added to verify correct parsing of quoted identifiers and default expressions across dialects.
This change adds integration tests to ExportPipelineIT and ImportPipelineIT to verify that User-Defined Functions (UDFs) are correctly exported and imported across different Spanner dialects (GoogleSQL and PostgreSQL). - Added UDF definitions to test DDL files. - Updated ExportPipelineIT to verify UDF artifacts in GCS. - Added testPostgresImportPipeline_UDF to ImportPipelineIT to verify UDF restoration from Avro artifacts. - Included necessary Avro and manifest resources for the import test.
- Replaced manual index manipulation with regex in UdfParameter.parse for better readability and robust dialect support. - Added support for PostgreSQL types with spaces (e.g., 'double precision') in UdfParameter.parse. - Preserved GoogleSQL single-word type restriction to minimize behavioral changes. - Added unit tests for PostgreSQL types with spaces. - Fixed unused import in InformationSchemaScannerTest.java.
- Delete pr_comments.json per PR review feedback. - Fix Ddl.Builder.createUdf to pass dialect to Udf.builder(dialect) so scanned PostgreSQL UDFs retain Dialect.POSTGRESQL. - Fix PostgreSQL UDF parameter quoting and determinism assertions in ExportPipelineIT. - Add missing pg_multiply.avro resource and fix manifest filenames and Base64 MD5 checksums for ImportPipelineIT. - Reintroduce PostgreSQL UDF coverage in CopyDbIT-AllSchemaAndData-pg.sql and AvroSchemaToDdlConverterTest. TAG=agy CONV=f1c413a3-3f8a-43cd-8eac-9b7a2df210e0
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces support for User-Defined Functions (UDFs) within the PostgreSQL dialect for Spanner Dataflow templates. It enables the export and import of PostgreSQL UDFs by mapping determinism levels to volatility keywords and enhancing parameter parsing to handle quoted identifiers and default values. These changes ensure consistency and compatibility when migrating or exporting Spanner databases using the PostgreSQL dialect. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for User-Defined Functions (UDFs) in the PostgreSQL dialect for Cloud Spanner, including metadata extraction, Avro schema conversion, and SQL generation. The review feedback highlights critical improvements to the parsing logic in UdfParameter.parse, such as fixing a regex bug where Pattern.quote is incorrectly used inside a character class, and addressing a split-logic issue that could bypass empty default expression checks. Additionally, suggestions are made to increase robustness by trimming input parameters and handling case-insensitivity for determinism values. All comments are highly actionable and should be addressed.
| String regex = | ||
| String.format( | ||
| "^((%1$s[^%1$s]+%1$s)|(\\S+))\\s+(.*)$", java.util.regex.Pattern.quote(quote)); |
There was a problem hiding this comment.
Using Pattern.quote(quote) inside the character class [^%1$s] is problematic because Pattern.quote wraps the string in \Q and \E. Inside a regex character class [...], \Q and \E are treated as literal characters in many regex engines or can cause unexpected matching behavior (e.g., failing to match identifiers containing 'Q' or 'E'). Since quote is always a single character (double quote or backtick), you can safely use it directly without quoting.
| String regex = | |
| String.format( | |
| "^((%1$s[^%1$s]+%1$s)|(\\S+))\\s+(.*)$", java.util.regex.Pattern.quote(quote)); | |
| String regex = | |
| String.format( | |
| "^((%1$s[^%1$s]+%1$s)|(\\S+))\\s+(.*)$", quote); |
| public static UdfParameter parse(String parameter, String functionSpecificName, Dialect dialect) { | ||
| String[] paramParts = parameter.split(" "); | ||
| if (paramParts.length < 2) { | ||
| String quote = identifierQuote(dialect); |
There was a problem hiding this comment.
Trimming the parameter string at the beginning of parse ensures that any leading or trailing whitespace is robustly handled, preventing unexpected parsing failures.
| public static UdfParameter parse(String parameter, String functionSpecificName, Dialect dialect) { | |
| String[] paramParts = parameter.split(" "); | |
| if (paramParts.length < 2) { | |
| String quote = identifierQuote(dialect); | |
| public static UdfParameter parse(String parameter, String functionSpecificName, Dialect dialect) { | |
| parameter = parameter.trim(); | |
| String quote = identifierQuote(dialect); |
| // Split 'rest' into type and defaultExpression using a case-insensitive 'DEFAULT' keyword. | ||
| String[] parts = rest.split("(?i)\\s+DEFAULT\\s+", 2); | ||
| String type = parts[0].trim(); | ||
| String defaultExpression = parts.length > 1 ? parts[1].trim() : null; |
There was a problem hiding this comment.
Using (?i)\s+DEFAULT\s+ as the split regex requires at least one whitespace character after the DEFAULT keyword. If a parameter ends with DEFAULT (e.g., p1 int32 DEFAULT), the regex won't match, resulting in defaultExpression being null and bypassing the empty default check. Changing the regex to (?i)\s+DEFAULT\b ensures it splits correctly even when there is no trailing expression, making the defaultExpression.isEmpty() validation reachable and robust.
| // Split 'rest' into type and defaultExpression using a case-insensitive 'DEFAULT' keyword. | |
| String[] parts = rest.split("(?i)\\s+DEFAULT\\s+", 2); | |
| String type = parts[0].trim(); | |
| String defaultExpression = parts.length > 1 ? parts[1].trim() : null; | |
| // Split 'rest' into type and defaultExpression using a case-insensitive 'DEFAULT' keyword. | |
| String[] parts = rest.split("(?i)\\s+DEFAULT\\b", 2); | |
| String type = parts[0].trim(); | |
| String defaultExpression = parts.length > 1 ? parts[1].trim() : null; |
| } else if (spannerDeterminism() != null && dialect() == Dialect.POSTGRESQL) { | ||
| switch (spannerDeterminism()) { |
There was a problem hiding this comment.
To ensure robustness against different casing of the determinism string (e.g., if retrieved from external sources or metadata), it is safer to perform a case-insensitive comparison by converting spannerDeterminism() to uppercase before matching.
| } else if (spannerDeterminism() != null && dialect() == Dialect.POSTGRESQL) { | |
| switch (spannerDeterminism()) { | |
| } else if (spannerDeterminism() != null && dialect() == Dialect.POSTGRESQL) { | |
| switch (spannerDeterminism().toUpperCase()) { |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4240 +/- ##
============================================
+ Coverage 55.85% 56.05% +0.20%
+ Complexity 7488 7135 -353
============================================
Files 1135 1139 +4
Lines 70313 70793 +480
Branches 8040 8091 +51
============================================
+ Hits 39271 39685 +414
- Misses 28490 28540 +50
- Partials 2552 2568 +16
🚀 New features to boost your workflow:
|
2 participants
Adding support for UDFs in the PostgreSQL dialect.
Note, this PR is based on the previously reviewed #3325.
This PR has been rebased, review comments addressed, and a few test failures addressed.