Skip to content

Conversation

@fbiville
Copy link
Collaborator

No description provided.

@fbiville fbiville force-pushed the neo4j/group_by_synthetic_fields branch 2 times, most recently from eb82917 to 67df9a2 Compare January 6, 2025 16:50
@pull-request-size pull-request-size bot added size/L and removed size/XS labels Jan 6, 2025
Copy link
Contributor

@venikkin venikkin 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!

@fbiville fbiville force-pushed the neo4j/group_by_synthetic_fields branch from 67df9a2 to 4aa7a3f Compare January 7, 2025 13:07
This is an old bug which could only surface with the more recent
addition of custom Cypher queries.

The template tries to pre-fetch source data.

It does it for text sources, since they do not support SQL
pushdown, so their data is required for post-processing anyway.

Before this commit, the template also pre-fetched data when none
of the source's targets defined source transformations.

This is overly restrictive and actually wrong.

Custom query targets cannot define source transformations.

If they share a source with a node/rel targets that define custom
source transformations, then the template would crash with a NPE.

This is now fixed. Source data is pre-fetched as long as there is
at least one of its target that does not define any transformation.

The commit also adds another small optimization: if the source
does not match any active targets, the source processing is
skipped completely. Before that, the data could be pre-fetched,
incurring unnecessary data movement.
@fbiville fbiville force-pushed the neo4j/group_by_synthetic_fields branch from 4aa7a3f to 1b6a2c5 Compare January 7, 2025 13:08
@fbiville fbiville changed the title fix: include synthetic fields in GROUP BY fix: eager source row fetching logic Jan 7, 2025
@fbiville fbiville marked this pull request as ready for review January 7, 2025 13:08
@codecov
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 0% with 22 lines in your changes missing coverage. Please review.

Project coverage is 46.57%. Comparing base (23be7bc) to head (1b6a2c5).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...eleport/v2/neo4j/templates/GoogleCloudToNeo4j.java 0.00% 22 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2071   +/-   ##
=========================================
  Coverage     46.57%   46.57%           
- Complexity     3957     3959    +2     
=========================================
  Files           869      869           
  Lines         51600    51599    -1     
  Branches       5404     5403    -1     
=========================================
+ Hits          24031    24033    +2     
+ Misses        25849    25847    -2     
+ Partials       1720     1719    -1     
Components Coverage Δ
spanner-templates 68.57% <ø> (+0.01%) ⬆️
spanner-import-export 65.57% <ø> (+0.03%) ⬆️
spanner-live-forward-migration 77.36% <ø> (ø)
spanner-live-reverse-replication 78.32% <ø> (ø)
spanner-bulk-migration 87.62% <ø> (ø)
Files with missing lines Coverage Δ
...ogle/cloud/teleport/v2/neo4j/utils/ModelUtils.java 64.03% <ø> (+1.64%) ⬆️
...eleport/v2/neo4j/templates/GoogleCloudToNeo4j.java 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

@Abacn Abacn merged commit 9d39b07 into GoogleCloudPlatform:main Jan 7, 2025
14 of 15 checks passed
@fbiville fbiville deleted the neo4j/group_by_synthetic_fields branch January 7, 2025 15:54
taherkl added a commit to ollionorg/DataflowTemplates-fork that referenced this pull request Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants