Skip to content

Add support for PropertyGraphs in import/export DDL#2003

Merged
darshan-sj merged 7 commits intoGoogleCloudPlatform:mainfrom
smukil:add_property_graph_ddl_support
Nov 29, 2024
Merged

Add support for PropertyGraphs in import/export DDL#2003
darshan-sj merged 7 commits intoGoogleCloudPlatform:mainfrom
smukil:add_property_graph_ddl_support

Conversation

@smukil
Copy link
Copy Markdown
Contributor

@smukil smukil commented Nov 11, 2024

No description provided.

@smukil smukil requested a review from a team as a code owner November 11, 2024 21:29
@smukil
Copy link
Copy Markdown
Contributor Author

smukil commented Nov 11, 2024

@darshan-sj As we spoke earlier, here's the first patch for PropertyGraph support in import/export. I will send the subsequent as I get them completed.

@smukil
Copy link
Copy Markdown
Contributor Author

smukil commented Nov 11, 2024

@asthamohta @shreyakhajanchi An fyi, you were auto-selected for review. I've been in conversation with @darshan-sj regarding this effort. But I don't seem to have the permissions to change the reviewer to him.

@smukil smukil force-pushed the add_property_graph_ddl_support branch 2 times, most recently from 05206b3 to 100c4e1 Compare November 14, 2024 02:42
@smukil
Copy link
Copy Markdown
Contributor Author

smukil commented Nov 14, 2024

The build should be fixed now. Turned out to be a dependency issue as OpenJDK 11 doesn't come with javaFX anymore (which Pair depends on). I updated and removed it.

@smukil smukil force-pushed the add_property_graph_ddl_support branch from 100c4e1 to a06acc9 Compare November 14, 2024 05:57
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 79.72665% with 89 lines in your changes missing coverage. Please review.

Project coverage is 45.88%. Comparing base (b04de34) to head (29d3f8e).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...google/cloud/teleport/spanner/ExportTransform.java 0.00% 20 Missing ⚠️
...google/cloud/teleport/spanner/ImportTransform.java 0.00% 18 Missing ⚠️
...ogle/cloud/teleport/spanner/ddl/PropertyGraph.java 83.87% 9 Missing and 6 partials ⚠️
.../cloud/teleport/spanner/ddl/GraphElementTable.java 84.78% 11 Missing and 3 partials ⚠️
...ava/com/google/cloud/teleport/spanner/ddl/Ddl.java 53.57% 8 Missing and 5 partials ⚠️
...oud/teleport/spanner/AvroSchemaToDdlConverter.java 92.96% 7 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2003      +/-   ##
============================================
+ Coverage     45.42%   45.88%   +0.46%     
- Complexity     3678     3775      +97     
============================================
  Files           842      849       +7     
  Lines         49970    50624     +654     
  Branches       5261     5322      +61     
============================================
+ Hits          22697    23231     +534     
- Misses        25605    25701      +96     
- Partials       1668     1692      +24     
Components Coverage Δ
spanner-templates 67.39% <79.72%> (+0.66%) ⬆️
spanner-import-export 65.42% <79.72%> (+1.19%) ⬆️
spanner-live-forward-migration 75.91% <ø> (+0.02%) ⬆️
spanner-live-reverse-replication 76.87% <ø> (+0.22%) ⬆️
spanner-bulk-migration 86.61% <ø> (+0.23%) ⬆️
Files with missing lines Coverage Δ
...va/com/google/cloud/teleport/spanner/AvroUtil.java 93.75% <ø> (ø)
...oud/teleport/spanner/DdlToAvroSchemaConverter.java 92.83% <100.00%> (+1.96%) ⬆️
...oud/teleport/spanner/AvroSchemaToDdlConverter.java 83.89% <92.96%> (+4.03%) ⬆️
...ava/com/google/cloud/teleport/spanner/ddl/Ddl.java 71.25% <53.57%> (-1.29%) ⬇️
.../cloud/teleport/spanner/ddl/GraphElementTable.java 84.78% <84.78%> (ø)
...ogle/cloud/teleport/spanner/ddl/PropertyGraph.java 83.87% <83.87%> (ø)
...google/cloud/teleport/spanner/ImportTransform.java 22.40% <0.00%> (-0.70%) ⬇️
...google/cloud/teleport/spanner/ExportTransform.java 16.00% <0.00%> (-0.70%) ⬇️

... and 34 files with indirect coverage changes

@smukil
Copy link
Copy Markdown
Contributor Author

smukil commented Nov 15, 2024

Applied mvn spotless. I don't seem to have the permissions to add the label. But I believe the label should be "improvement" for this.

@darshan-sj

@smukil smukil force-pushed the add_property_graph_ddl_support branch from 8b748a8 to 9905704 Compare November 19, 2024 23:02
@smukil smukil force-pushed the add_property_graph_ddl_support branch from 9905704 to 0ef44bb Compare November 19, 2024 23:15
@smukil
Copy link
Copy Markdown
Contributor Author

smukil commented Nov 20, 2024

@darshan-sj Added Graph INFORMATION SCHEMA patch which populates Ddl.PropertyGraph by parsing information schema. One more patch pending for Import/ExportTransform, which I will push shortly.

@smukil smukil force-pushed the add_property_graph_ddl_support branch from 9f3459e to 63b1961 Compare November 20, 2024 21:42
@smukil
Copy link
Copy Markdown
Contributor Author

smukil commented Nov 21, 2024

@darshan-sj I've pushed the code for import/export of graphs as well. I tested it and was able to export a graph from one DB and import it into another. I also tried exporting multiple graphs at the same time and it works.

As mentioned in our other chat, the only remaining issue is that the ImportFromAvroTest for graph fails with a cryptic error inside Beam, so I've commented it out for now, but will reenable it once we get to the bottom of that issue.

For all other purposes, DataflowTemplates for graph works well.

@smukil smukil requested a review from darshan-sj November 27, 2024 22:04
@darshan-sj darshan-sj added the improvement Making existing code better label Nov 28, 2024
darshan-sj
darshan-sj previously approved these changes Nov 28, 2024
Copy link
Copy Markdown
Contributor

@darshan-sj darshan-sj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for one [Must fix] comment others comments are nits. Feel free to address them in subsequent PRs.

Overall changes look good!

@smukil smukil force-pushed the add_property_graph_ddl_support branch from 715d4ee to 29d3f8e Compare November 29, 2024 01:04
Copy link
Copy Markdown
Contributor

@darshan-sj darshan-sj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good!

@darshan-sj
Copy link
Copy Markdown
Contributor

The IT failure is not related to the code changes in this PR.

@darshan-sj darshan-sj merged commit 9854a70 into GoogleCloudPlatform:main Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Making existing code better size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants