Repository navigation
PostgreSQL UDF Support - #4240
Conversation
|
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4240 +/- ##
============================================
- Coverage 62.66% 56.99% -5.67%
- Complexity 3570 7625 +4055
============================================
Files 607 1161 +554
Lines 35213 73915 +38702
Branches 4001 8704 +4703
============================================
+ Hits 22067 42130 +20063
- Misses 12026 28983 +16957
- Partials 1120 2802 +1682
🚀 New features to boost your workflow:
|
…ameter - Trim parameter string at the start of UdfParameter.parse. - Pass quote character directly in UdfParameter.parse regex. - Use word boundary (?i)\s+DEFAULT\b when splitting DEFAULT expression in UdfParameter.parse. - Match spannerDeterminism() case-insensitively in Udf.prettyPrint. - Add unit tests in UdfParameterTest and UdfTest. TAG=agy CONV=f1c413a3-3f8a-43cd-8eac-9b7a2df210e0
30389f3 to
41c7fbb
Compare
2829b43 to
968c062
Compare
bharadwaj-aditya
left a comment
bharadwaj-aditya
left a comment
There was a problem hiding this comment.
Overall, this looks very clean! A few minor comments on edge cases, test consolidation, and the cla/google check:
cla/googlecheck: The earlier commits on this branch are authored byjacobwyant@google.com(@MnkyGns), which is causing thecla/googlecheck to fail. Squashing the commits under@c2nesauthorship (or updating commit authorship) should resolve the CLA failure.
|
@bharadwaj-aditya please take a look. I believe all your comments have been addressed and all checks have passed. |
darshan-sj
left a comment
darshan-sj
left a comment
There was a problem hiding this comment.
Looks good overall. Left a few comments.
Export and import SQL user-defined functions for PostgreSQL-dialect
databases. Previously these were GoogleSQL only.
InformationSchemaScanner:
- Scan PostgreSQL routines and parameters: spanner_type,
spanner_determinism and the qualified specific_schema.
- Skip EXTERNAL routines, e.g. change stream read_json_* functions.
- Gate UDF export on information_schema.routines.spanner_determinism
being present.
Ddl / Udf / UdfParameter:
- Ddl.Builder.createUdf propagates the dialect.
- Udf.prettyPrint emits PostgreSQL syntax: IMMUTABLE/STABLE/VOLATILE
from spanner_determinism (case-insensitive), SECURITY without SQL, and
RETURN <expr> bodies.
- Never print LANGUAGE SQL for SQL UDFs. ROUTINE_BODY reports SQL, but
neither dialect accepts LANGUAGE SQL with a SQL body. PostgreSQL has
rejected it since --spangres_enable_remote_udf defaulted to true.
- UdfParameter.parse is dialect-aware:
- quoted or unquoted names;
- types containing spaces in both dialects (PostgreSQL multi-word types,
GoogleSQL STRUCT<...>, which previously failed to import);
- case-insensitive DEFAULT;
- multi-line DEFAULT expressions;
- names stored unquoted, so Ddl -> Avro -> Ddl round-trips match the
scanner.
Avro:
- Serialize spanner_determinism as spannerUdfSpannerDeterminism.
Tests:
- Unit tests in UdfTest, UdfParameterTest, DdlTest,
InformationSchemaScannerTest, AvroSchemaToDdlConverterTest and
DdlToAvroSchemaConverterTest. RandomDdlGenerator now generates PG UDF
determinism.
- InformationSchemaScannerIT covers PG UDFs, including determinism.
- ExportPipelineIT exports PG UDFs and checks the Avro properties.
- ImportPipelineIT imports pg_add and pg_multiply as part of
testPostgresImportPipeline and calls them.
- CopyDbIT covers PG UDF parameter and return types (scalar, multi-word,
array), mixed-case names, literal defaults, no parameters, and every
volatility (IMMUTABLE, STABLE, VOLATILE, unspecified). The GSQL file
adds a UDF with STRUCT and ARRAY<STRUCT> parameters.
- UDFs live in named schemas, because Cloud Spanner disallows UDFs in the
default schema.
Based on GoogleCloudPlatform#3325 by Jacob Wyant.
TAG=agy
CONV=76f8ea42-8008-4a45-8c97-887632526bf8
|
The integration test failure was in an unrelated Neo4j test case. |
c4e24f0
into
GoogleCloudPlatform:main
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.