Feat!: Adjust physical_properties evaluation and add macro to resolve physical table names

[erindru]

This PR:

• Moves evaluation of physical_properties to run time instead of load time
  • This enables the usage of things that only exist at run time, such as @this_model
• Adds a macro called @physical_location that allows a user to create arbitrary exp.Literal or exp.Table objects in the AST based on the parts of the physical table name

The motivation is twofold:

• Right now, there is no way to set custom locations for tables on engines that separate storage and compute (Athena, Trino, Spark etc). This is true even if you write a custom macro and reference it from physical_properties, because physical_properties is currently rendered at load time which means no snapshot info is available
• Right now, @this_model returns a fully rendered FQN string which makes it difficult to work with. We can't change this without breaking backwards compatibility, but we can introduce something else that allows users to build new strings based on the components of the FQN, rather than the FQN itself

[erindru]

As always, naming things is one of the hardest problems. I considered:

• @physical_table - the object being created is not always a table, such as kind: view
• @this_snapshot - uses the terminology snapshot which I can see in some places is being eschewed for "model version"
• @this_model_version - kind of long winded

[themisvaltinos]

Perhaps this could be moved inside apply, so it doesn't occur when limit is None where the properties aren't needed. Though this doesn't seem a particularly expensive render, so it's likely not that significant

[erindru]

Oh, the reason I had it outside apply was so that we didnt occur the hit for every index being applied since we only need to render it once.

I see what you're saying though. Perhaps it could be rendered right after the limit check and passed to apply instead? The problem is, apply is called in a bunch of places so all the call sites would need to be updated

[themisvaltinos]

Yes you're right, I hadn't considered that. Maybe then something like this:

rendered_physical_properties = snapshot.model.render_physical_properties(
    **render_statements_kwargs
) if limit is None else None

But arguably I don't think it is that much of a slowdown, so the way you have it now should be fine

[themisvaltinos]

Small typo: Generates either a String literal...

[erindru]

Thanks!

[izeigerman]

Don't we want the same behavior for virtual properties?

[erindru]

Recording the result of our internal discussion here: maybe, but I couldnt think of a use-case and was trying to keep this PR small.

Since the load time rendering context is a subset of the runtime rendering context, if we were to later find a use-case to render virtual_properties at runtime then we could make the switch then without breaking backwards compatibility

[izeigerman]

@tobymao does this API look good to you?

[izeigerman]

IMHO, the name is somewhat confusing and doesn't accurately reflect what it does. We already have resolve_table can this be resolve_template? Or perhaps we can extend resolve_table with an optional template instead? If it's provided then we follow the template, if not we return fqn.

[erindru]

Yep, I don't like the current name either.

The problem with extending resolve_table is that it's not a macro per se - it's a lambda that gets passed into the template context. And it's also available directly in Python from Python models and is defined to return a str instead of an AST node so I think changing it will cause breakage.

I like @resolve_template - it doesnt limit the terminiology to tables only (since it's really "physical layer object" which could be a view for kind: VIEW models) and it indicates that templating is occurring

[tobymao]

ok this is fine

[georgesittas]

FYI, there's also the "testing" runtime stage. How would these macro calls be handled in the context of unit tests? Can we actually unit test models that use this?

[erindru]

Good call, i'll write a test to verify the behaviour during unit tests.

Theoretically it should be ignored entirely unless @this_model is available so maybe I adjust the condition to just check for the presence of that and ignore the runtime stage

[erindru]

I've adjusted the logic to be dependent on the presence of @this_model instead and written a test to show it works from unit tests

[georgesittas]

[Nit]

Suggested change

	else:
	return exp.Literal.string(result)
	return exp.Literal.string(result)

[georgesittas]

Should we pass the dialect in this to_table call?

[erindru]

Man this trap for young players always gets me 🤦

Thanks for noticing it, will fix

[georgesittas]

I think this is breaking because it impacts the data hash. If someone used @gateway or other load-time-renderable variables within physical_properties, then they should get a diff due to this change.

This is similar to the recent when_matched refactor, IIRC.

[erindru]

Ah, yes, I was meant to ask about this.

I'm still not 100% with how to write migrations and how to test they're working, they seem to be quite difficult / error prone to write with lots of pitfalls.

Also, is there any default upgrade logic that we get "for free" with a sqlglot version change?

[erindru]

I spoke with @izeigerman and there isnt a way to migrate this automatically.

The problem is that the state would contain already-rendered macros and we wouldnt know what the unrendered version would look like without the project files.

When a migration occurs, access to the project files is not guaranteed. So we are just hoping that not many people were using macros / variables in physical_properties.

The workaround if this triggers a bunch of metadata changes and tries to rebuild everything would be to run a --forward-only plan to update state but not rebuild any tables

[georgesittas]

Sounds good– sorry for not getting back to this thread earlier.

[tobymao]

why template? i'm not sure i understand the name, seems a bit too generic

[erindru]

maybe @render_template would be more appropriate?

It's essentially rendering an in-line template with @this_model broken out into its components and made available as @{catalog_name}, @{schema_name} and @{table_name} so the user can recombine those as required

My original implementation added these as variables to the main evaluation context so users could access them directly, something like:

physical_properties (
  location = @'s3://bucket/@{catalog_name}/@{schema_name}/@{table_name}'
)

but there was a preference to create a macro instead?

[tobymao]

is this just doing string replacement?, is this safe?

[erindru]

In short, yes. It's doing the same thing that this is doing to keep consistency. It deliberately mimics our macro syntax, is that what you meant by safety?

I was holding off "upgrading" it to actually run the template string through the macro evaluator via evaluator.evaluate(), figuring that could be easily added later if the need arose

[tobymao]

this can just be a literal string, the type convert will handle it

mode: str