feat(hydro_lang): add global location ID, name free variables #2389
Conversation
Deploying hydro with
|
| Latest commit: |
d6e30bd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://993a0292.hydroflow.pages.dev |
| Branch Preview URL: | https://global-location.hydroflow.pages.dev |
MingweiSamuel
changed the title
LocationId variableThere was a problem hiding this comment.
Pull request overview
This PR adds a name() method to the Location trait that returns a human-readable string for debugging and telemetry purposes. It also introduces two new free variables, LOCATION_SELF_ID and LOCATION_SELF_NAME, that can be used in quoted code snippets to access the location's ID and name at runtime.
Key changes:
- Added
name()method to theLocationtrait that returns aStringwith a human-readable name - Implemented
name()for all location types:Process,Cluster,Tick, andAtomic - Added
LOCATION_SELF_IDandLOCATION_SELF_NAMEfree variables with their correspondingFreeVariableWithContextimplementations
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| hydro_lang/src/location/mod.rs | Adds the name() method to the Location trait and implements two new free variables (LOCATION_SELF_ID and LOCATION_SELF_NAME) for accessing location metadata in quoted code |
| hydro_lang/src/location/process.rs | Implements name() method for Process location type, returning a formatted string with the process type |
| hydro_lang/src/location/cluster.rs | Implements name() method for Cluster location type, returning a formatted string with the cluster type |
| hydro_lang/src/location/tick.rs | Implements name() method for both Tick and Atomic location types, composing names from their wrapped location types |
| hydro_lang/src/compile/trybuild/generate.rs | Reformats the __hydro_runtime function signature for better readability (no functional change) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| where | ||
| L: Location<'a>, | ||
| { | ||
| type O = LocationId; |
There was a problem hiding this comment.
The type O for LocationSelfName is incorrectly set to LocationId. According to the documentation comment on line 864, this free variable should turn into a &'static str, not a LocationId. This type mismatch could cause compilation errors or runtime issues when the free variable is used in quoted code. The type should be &'static str to match the documented behavior and the actual value being generated (a string literal).
| type O = LocationId; | |
| type O = &'static str; |
| let location_name = <L::Root as Location>::name(); | ||
|
|
||
| QuoteTokens { | ||
| prelude: None, | ||
| expr: Some(quote! { #location_name }), |
There was a problem hiding this comment.
The quote! { #location_name } macro invocation will not generate a string literal as expected. When location_name is a String, the quote macro interpolates it as a token stream, which will likely result in invalid Rust code. To generate a string literal in the quoted code, you should convert the String to a syn::LitStr first, for example: let location_name = syn::LitStr::new(&<L::Root as Location>::name(), proc_macro2::Span::call_site()); and then use quote! { #location_name }.
| } | ||
|
|
||
| /// A free variable representing the root location's human-readable name. When spliced in | ||
| /// a quoted snippet that will run on a cluster, this turns into a [`&'static str`]. |
There was a problem hiding this comment.
The documentation comment incorrectly uses backticks around &'static str which creates a broken link. Since this is a primitive type reference, it should be written in regular code format without the link syntax, or use plain text. Consider changing to "When spliced in a quoted snippet that will run on a cluster, this turns into a string literal."
| /// a quoted snippet that will run on a cluster, this turns into a [`&'static str`]. | |
| /// a quoted snippet that will run on a cluster, this turns into a string literal. |
|
Need to add tests still |
|
what do you forsee are the uses for the location id? For logging/metrics? I don't think there's any guarantees around the stability of this ID from one compilation to another. |
|
Yeah for the first pass of metrics we need a way to differentiate between different locations as right now so metrics for subgraph 1 in the e.g. leader don't get aggregated with subgraph 1 in the client. So although its preferable if they're stable, they just need to be distinct |
No description provided.