Add UDF support to import/export workflows #2253

MnkyGns · 2025-03-17T14:38:36Z

Adding support for User Defined Functions (UDFs) to be imported from and exported to Avro files. Supported format follows the syntax -

CREATE FUNCTION
  function_name ([ parameter_name data_type [DEFAULT default_value], ...])
  [ RETURNS data_type ]
  [ SQL SECURITY INVOKER ]
  AS ( function_body )

Hoping to fix test failures in v2/

codecov · 2025-03-25T16:09:50Z

Codecov Report

Attention: Patch coverage is 59.13043% with 94 lines in your changes missing coverage. Please review.

Project coverage is 48.87%. Comparing base (095c8d4) to head (375a8ab).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
...ava/com/google/cloud/teleport/spanner/ddl/Udf.java	63.63%	27 Missing and 1 partial ⚠️
...google/cloud/teleport/spanner/ImportTransform.java	0.00%	21 Missing and 2 partials ⚠️
...google/cloud/teleport/spanner/ExportTransform.java	9.52%	19 Missing ⚠️
...oogle/cloud/teleport/spanner/ddl/UdfParameter.java	64.28%	12 Missing and 3 partials ⚠️
...ava/com/google/cloud/teleport/spanner/ddl/Ddl.java	73.07%	4 Missing and 3 partials ⚠️
...oud/teleport/spanner/AvroSchemaToDdlConverter.java	94.73%	0 Missing and 1 partial ⚠️
...oogle/cloud/teleport/spanner/common/NameUtils.java	0.00%	0 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (59.13%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2253      +/-   ##
============================================
+ Coverage     48.83%   48.87%   +0.03%     
- Complexity     4514     4542      +28     
============================================
  Files           905      907       +2     
  Lines         55061    55290     +229     
  Branches       5881     5921      +40     
============================================
+ Hits          26891    27024     +133     
- Misses        26251    26335      +84     
- Partials       1919     1931      +12

Components	Coverage Δ
spanner-templates	`69.69% <59.13%> (-0.14%)`	⬇️
spanner-import-export	`67.77% <59.13%> (-0.28%)`	⬇️
spanner-live-forward-migration	`77.64% <ø> (ø)`
spanner-live-reverse-replication	`79.69% <ø> (ø)`
spanner-bulk-migration	`88.39% <ø> (ø)`

Files with missing lines	Coverage Δ
...va/com/google/cloud/teleport/spanner/AvroUtil.java	`93.75% <ø> (ø)`
...oud/teleport/spanner/DdlToAvroSchemaConverter.java	`93.63% <100.00%> (+0.45%)`	⬆️
...oud/teleport/spanner/AvroSchemaToDdlConverter.java	`85.17% <94.73%> (+0.44%)`	⬆️
...oogle/cloud/teleport/spanner/common/NameUtils.java	`55.17% <0.00%> (-3.45%)`	⬇️
...ava/com/google/cloud/teleport/spanner/ddl/Ddl.java	`71.36% <73.07%> (+0.10%)`	⬆️
...oogle/cloud/teleport/spanner/ddl/UdfParameter.java	`64.28% <64.28%> (ø)`
...google/cloud/teleport/spanner/ExportTransform.java	`15.73% <9.52%> (-0.28%)`	⬇️
...google/cloud/teleport/spanner/ImportTransform.java	`21.41% <0.00%> (-0.99%)`	⬇️
...ava/com/google/cloud/teleport/spanner/ddl/Udf.java	`63.63% <63.63%> (ø)`

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

darshan-sj

Changes look good overall. Left few comments, Please take a look.

darshan-sj · 2025-04-03T06:29:45Z

v1/src/main/java/com/google/cloud/teleport/spanner/AvroSchemaToDdlConverter.java

@@ -143,6 +153,31 @@ public View toView(String viewName, Schema schema) {
    return builder.build();
  }

+  public Udf toUdf(String functionSpecificName, Schema schema) {


What is functionSpecificName?

That would be the full name, including the schema/namespace prefix

darshan-sj · 2025-04-03T06:32:37Z

v1/src/main/java/com/google/cloud/teleport/spanner/DdlToAvroSchemaConverter.java

+      recordBuilder.prop(SPANNER_NAME, udf.specificName());
+      recordBuilder.prop(GOOGLE_FORMAT_VERSION, version);
+      recordBuilder.prop(GOOGLE_STORAGE, "CloudSpanner");
+      // Indicate that this is a "CREATE FUNCTION", not a table or a view.


Just checking, is this a TODO comment?

No, it's just pointing out that this property is to distinguish the type of object

darshan-sj · 2025-04-03T06:37:51Z

v1/src/main/java/com/google/cloud/teleport/spanner/ImportTransform.java

@@ -490,6 +491,7 @@ public void processElement(ProcessContext c) {
                          List<KV<String, Schema>> missingViews = new ArrayList<>();
                          List<KV<String, Schema>> missingChangeStreams = new ArrayList<>();
                          List<KV<String, Schema>> missingSequences = new ArrayList<>();
+                          List<KV<String, Schema>> missingFunctions = new ArrayList<>();


Since we are calling it UDF in other places, Lets call it UDFs everywhere? rename to missingUDFs

darshan-sj · 2025-04-03T06:41:34Z

v1/src/main/java/com/google/cloud/teleport/spanner/ImportTransform.java

+                              mergedDdl.addUdf(udf);
+                            }
+                            Ddl newDdl = builder.build();
+                            ddlStatements.addAll(newDdl.createUdfStatements());


I see that the missingTables and views are getting added to ddlStatements in Next lines. If UDFs would depend on tables, views etc, they should added to ddlStatements later. The order of statements in ddlStatements matters.

This is intentional, the plan is to initially only support "UDF_without_SELECT"

https://docs.google.com/document/d/1FeMWmvvQzymWhceNawC_QLmarZe9mDK408RQax7eiSw/edit?pli=1&tab=t.0#bookmark=id.k49vu7dbcae7

darshan-sj · 2025-04-03T07:25:01Z

v1/src/main/java/com/google/cloud/teleport/spanner/ImportTransform.java

@@ -798,6 +817,12 @@ public void processElement(ProcessContext c) {
                          c.output(KV.of(sequence.getName(), fullPath));
                        }
                      }
+                      for (Export.Table udf : proto.getFunctionsList()) {


The proto here is Export.proto is it? We should call it udf consistently everywhere.

darshan-sj · 2025-04-03T07:57:33Z

v1/src/main/java/com/google/cloud/teleport/spanner/ddl/InformationSchemaScanner.java

@@ -1004,6 +1008,112 @@ private void listViews(Ddl.Builder builder) {
    }
  }

+  private void listFunctions(Ddl.Builder builder) {


Please confirm that all the information schema table names, and columns used in the queries here are present in production. Also, can you get the changes in this file reviewed by one of your team members?

Confirmed, and @seanfox agreed to review these changes

darshan-sj · 2025-04-03T08:01:32Z

v1/src/main/java/com/google/cloud/teleport/spanner/ddl/InformationSchemaScanner.java

@@ -1004,6 +1008,112 @@ private void listViews(Ddl.Builder builder) {
    }
  }

+  private void listFunctions(Ddl.Builder builder) {


Also, please add unit tests for changes in this file. In InformationSchemaScannerTest.java

Added testListFunctionParametersSQL

darshan-sj · 2025-04-03T08:03:35Z

v1/src/main/java/com/google/cloud/teleport/spanner/ddl/Udf.java

+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.google.cloud.teleport.spanner.ddl;


Please add unit tests and cover all the lines in this file.

Added UdfTest.java

darshan-sj · 2025-04-03T08:04:48Z

v1/src/main/java/com/google/cloud/teleport/spanner/ddl/UdfParameter.java

@@ -0,0 +1,136 @@
+/*


Please add unit tests and cover all the lines in this file.

Added UdfParameterTest.java

darshan-sj · 2025-04-03T08:06:24Z

v1/src/main/proto/export.proto

@@ -30,7 +30,7 @@ message Export {
  // change stream or sequence name. Both `data_files` and `manifest_file` can
  // be used to locate the corresponding data files.
  message Table {
-    // The name of the table, view, change stream or sequence.
+    // The name of the table, view, change stream, sequence, or function.


Please call this as udf for consistency.

MnkyGns added 6 commits March 11, 2025 13:57

jacobwyant: first commit of UDF support

89626e0

jacobwyant - rest of UDF changes

6740c04

Revert SpannerServerResource.java test changes

c4b7f3e

Adding transform logic and corresponding tests, including CopyDbtest

75e745a

minor test fix and maven spotless formatting fixes

1a65ef4

Merge remote-tracking branch 'origin/main' into new-branch

375a8ab

Hoping to fix test failures in v2/

MnkyGns requested a review from a team as a code owner March 17, 2025 14:38

MnkyGns requested review from manitgupta and bharadwaj-aditya March 17, 2025 14:38

pull-request-size bot added the size/XXL label Mar 17, 2025

darshan-sj added the improvement label Mar 25, 2025

darshan-sj reviewed Apr 3, 2025

View reviewed changes

Changes in response to PR comments

560b22a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UDF support to import/export workflows #2253

Add UDF support to import/export workflows #2253

MnkyGns commented Mar 17, 2025

codecov bot commented Mar 25, 2025

darshan-sj left a comment

darshan-sj Apr 3, 2025

MnkyGns Apr 4, 2025

darshan-sj Apr 3, 2025

MnkyGns Apr 4, 2025

darshan-sj Apr 3, 2025

MnkyGns Apr 4, 2025

darshan-sj Apr 3, 2025

MnkyGns Apr 4, 2025

darshan-sj Apr 3, 2025

MnkyGns Apr 8, 2025

darshan-sj Apr 3, 2025

MnkyGns Apr 8, 2025

darshan-sj Apr 3, 2025

MnkyGns Apr 8, 2025

darshan-sj Apr 3, 2025

MnkyGns Apr 8, 2025

darshan-sj Apr 3, 2025

MnkyGns Apr 8, 2025

darshan-sj Apr 3, 2025

MnkyGns Apr 8, 2025

Add UDF support to import/export workflows #2253

Are you sure you want to change the base?

Add UDF support to import/export workflows #2253

Conversation

MnkyGns commented Mar 17, 2025

codecov bot commented Mar 25, 2025

Codecov Report

darshan-sj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment