Skip to content

chore(query): replace UUIDs with canonical keys and add observers#2923

Merged
barakmich merged 4 commits intomainfrom
barakmich/remove_uuid
Feb 26, 2026
Merged

chore(query): replace UUIDs with canonical keys and add observers#2923
barakmich merged 4 commits intomainfrom
barakmich/remove_uuid

Conversation

@barakmich
Copy link
Contributor

@barakmich barakmich commented Feb 25, 2026

Description

Testing

References

@barakmich barakmich requested a review from a team as a code owner February 25, 2026 20:12
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Feb 25, 2026
@codecov
Copy link

codecov bot commented Feb 25, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 89.35574% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.45%. Comparing base (b7f9dd7) to head (4399d70).

Files with missing lines Patch % Lines
pkg/query/trace.go 84.41% 14 Missing and 3 partials ⚠️
pkg/query/observer_analyze.go 90.65% 11 Missing and 2 partials ⚠️
pkg/query/datastore.go 50.00% 4 Missing ⚠️
pkg/query/outline.go 66.67% 1 Missing and 1 partial ⚠️
pkg/query/recursive_sentinel.go 71.43% 1 Missing and 1 partial ⚠️

❌ Your project check has failed because the head coverage (74.45%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #2923       +/-   ##
===========================================
+ Coverage   49.07%   74.45%   +25.39%     
===========================================
  Files         420      488       +68     
  Lines       54148    60194     +6046     
===========================================
+ Hits        26569    44814    +18245     
+ Misses      24863    12232    -12631     
- Partials     2716     3148      +432

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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.

tstirrat15
tstirrat15 previously approved these changes Feb 25, 2026
View reviewed changes
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

The question about the reversed arrow iterator is the only thing I'm concerned about

pkg/query/analyze.go Outdated

// Write iterator name and ID (truncated for readability)
fmt.Fprintf(sb, "%s (ID: %s)\n", explain.Info, iterID[:8])
fmt.Fprintf(sb, "%s (Hash: %016x)\n", explain.Info, iterID)
Copy link
Contributor

@tstirrat15 tstirrat15 Feb 25, 2026

Choose a reason for hiding this comment

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

is this the format string for "base 16?"

Copy link
Contributor Author

@barakmich barakmich Feb 25, 2026

Choose a reason for hiding this comment

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

016x == 0-padded 16-length (which, for hex bytes is correct; 8 bytes of two hex characters) and the x is hexadecimal

pkg/query/datastore.go Outdated
return func(yield func(Path, error) bool) {
var cursor *tuple.Relationship
iteratorID := r.ID() + ":iter_subjects"
iteratorID := fmt.Sprintf("%x:iter_subjects", r.Hash())
Copy link
Contributor

@tstirrat15 tstirrat15 Feb 25, 2026

Choose a reason for hiding this comment

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

What's x vs 016x?

Copy link
Contributor Author

@barakmich barakmich Feb 25, 2026

Choose a reason for hiding this comment

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

yeah, let's make this one match

pkg/query/optimize_arrow_direction.go
left: a.left,
right: a.right,
direction: rightToLeft,
canonicalKey: a.canonicalKey,
Copy link
Contributor

@tstirrat15 tstirrat15 Feb 25, 2026

Choose a reason for hiding this comment

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

Is the direction of the iterator not a part of the set of args for the purposes of canonicalization?

Copy link
Contributor Author

@barakmich barakmich Feb 25, 2026

Choose a reason for hiding this comment

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

Correct. We don't care which way the arrow points to evaluate the set, the set is the same either way (and so canonical)

Copy link
Contributor

@tstirrat15 tstirrat15 Feb 25, 2026

Choose a reason for hiding this comment

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

Makes sense!

@barakmich barakmich force-pushed the barakmich/remove_uuid branch from efa88a2 to fecf50a Compare February 25, 2026 20:31
@barakmich barakmich enabled auto-merge (squash) February 25, 2026 20:32
tstirrat15
tstirrat15 previously approved these changes Feb 25, 2026
View reviewed changes
@barakmich barakmich force-pushed the barakmich/remove_uuid branch from fecf50a to 1d9126d Compare February 25, 2026 21:08
tstirrat15
tstirrat15 previously approved these changes Feb 25, 2026
View reviewed changes
auto-merge was automatically disabled February 25, 2026 21:12

Pull Request is not mergeable

@barakmich barakmich enabled auto-merge (squash) February 25, 2026 23:24
@barakmich barakmich disabled auto-merge February 25, 2026 23:31
@barakmich barakmich force-pushed the barakmich/remove_uuid branch from cd920bb to 4399d70 Compare February 25, 2026 23:32
tstirrat15
tstirrat15 approved these changes Feb 26, 2026
View reviewed changes
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

I like the observer changes, but have some questions about thread safety

@barakmich barakmich changed the title chore(query): replace UUIDs with hashes of canonical keys chore(query): replace UUIDs with canonical keys and add observers Feb 26, 2026
@barakmich barakmich force-pushed the barakmich/remove_uuid branch from 4399d70 to 772b71c Compare February 26, 2026 18:49
@barakmich barakmich enabled auto-merge (squash) February 26, 2026 18:51
@barakmich barakmich merged commit 96f92c7 into main Feb 26, 2026
103 of 109 checks passed
@barakmich barakmich deleted the barakmich/remove_uuid branch February 26, 2026 19:36
@github-actions github-actions bot locked and limited conversation to collaborators Feb 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tstirrat15 tstirrat15 tstirrat15 approved these changes

Assignees

No one assigned

Labels

area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) Skip-Changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@barakmich @tstirrat15