Feat: Add virtual properties and rename table_properties to physical_properties by Kayrnt · Pull Request #2561 · SQLMesh/sqlmesh

Kayrnt · 2024-05-03T17:00:20Z

closes #2014

A new property virtual_properties is introduced: it is a dictionary that propagate to the engine the provided properties to the virtual layer (the "target view")
physical_properties replaces table_properties (we keep the value as an alias for backward compatibility)
A deprecation warning will be issued to use physical_propertiesinstead of table_properties

To be figured out:

What should we do in the case of dbt adapter? Should we apply the label to:
- Just the physical layer
- Just the virtual layer
- Both
  ? So far, it's just on the physical layer.

sqlmesh/core/model/common.py

tobymao · 2024-05-03T17:06:50Z

sqlmesh/core/model/meta.py

can we remove this and do a migration to move all physical_properties in the database to virtual_properties?

but accept models in the file system from the model file to map to physical_properties?

https://github.com/TobikoData/sqlmesh/tree/main/sqlmesh/migrations

Isn't these fields meant for Pydantic field parsing?
My goal is to be able to parse:

table_properties ("deprecated")

physical_properties (the new table_properties)

virtual_properties (the new properties for virtual layer)

Should that append with a custom Field to factor table_propertiesand physical_properties.

can we remove this and do a migration to move all physical_properties in the database to virtual_properties?

Do you mean all table_properties to physical_properties to migrate?

so i imagine pydantic will only have two fields, physical/virtual_properties. we migrate in the state all usage of table_properties -> physical properties.

at parse time, if we detect table_properties, we log a message and remap that to physical_properties

I don't think we need to migrate anything. Can't we just add a table_properties alias to the physical_properties_ field?

If not, we can also handle this in the root model validator. And just pop table_properties, emit a warning, and then set it as physical_properties

In any case, only physical_properties should stay, and no updates should be necessary to the EngineAdapter layer. EngineAdapter shouldn't care whether it's "physical" or "virtual". Different level of abstraction.

I also don't think a migration is needed or maybe I'm missing?
@izeigerman, in this commit I've updated the Pydantic parsing to use both with the related warning. Isn't it what you suggest?
Regarding EngineAdapter, I think we could indeed skip the physical / virtual naming and skip to the actual materialization related naming: table_properties / view_properties WDYT?

In this commit I tried to set proper boundaries with physical/virtual until the evaluator that translate those into table/view (depending on the actual chosen materialization).

Hey @Kayrnt, after additional consideration, I've realized that you do need the migration after all. Since we're getting rid of the table_properties field altogether we do need to reflect this in the existing state.

The fact that you had to change the fixtures/migrations/snapshots.json file confirms this suspicion. If we don't migrate, the next time users run the plan command after migration, it will show the diff because the field name has changed.

You can test this by first initializing a project with duckdb using a previous SQLMesh version and then upgrading to your current branch and running the plan command.

Sorry about the confusion initially, I really hoped we could avoid it.

tobymao · 2024-05-03T17:09:24Z

is the error due to a new version of pydantic?

Kayrnt · 2024-05-03T17:19:19Z

is the error due to a new version of pydantic?

No I think it's PR code related as it works if I just use physical_properties. I suspect it's in the common code you pointed out 👀

tobymao · 2024-05-03T21:47:33Z

tests/core/test_model.py

let's use the builder api for the tests,

exp.array("('test-label', 'label-value')")

Right, it's more compact 👍

tobymao · 2024-05-04T05:58:48Z

tests/core/engine_adapter/test_bigquery.py

can you use sqlglot builder here ?

Sure, then I'll update the existing code too to be consistent

tobymao · 2024-05-04T05:59:16Z

tests/core/test_model.py

tobymao · 2024-05-04T05:59:21Z

tests/core/test_model.py

tobymao · 2024-05-04T06:00:18Z

tests/fixtures/migrations/snapshots.json

what changed here?

table_properties was renamed to physical_properties and since this is a single line of JSON the whole thing is included in the diff.

I think this file shouldn't have changed. The fact that it did indicates that a migration script is necessary.

I think we could keep table_properties as the Pydantic model is backward compatible (unless it's used another way?).
I didn't look into migrations so far so I'm not sure how it works and what's needed.

tobymao · 2024-05-04T06:01:08Z

seems almost ready, @izeigerman can you take a final look

georgesittas

LGTM as well, a few small suggestions.

georgesittas · 2024-05-04T11:11:44Z

docs/concepts/models/overview.md

Suggested change

- A key-value of arbitrary table properties specific to the target engine applied on SQLMesh physical layer. For example:

- A key-value mapping of arbitrary table properties specific to the target engine applied on SQLMesh's physical layer. For example:

georgesittas · 2024-05-04T11:11:54Z

docs/concepts/models/overview.md

Suggested change

- A key-value of arbitrary table properties specific to the target engine applied on SQLMesh virtual layer. For example:

- A key-value mapping of arbitrary table properties specific to the target engine applied on SQLMesh's virtual layer. For example:

georgesittas · 2024-05-04T11:12:28Z

docs/reference/model_configuration.md

Suggested change

| `physical_properties` | Arbitrary table properties specific to the target engine applied on SQLMesh physical layer. Specified as key-value pairs (`key = value`)

| `virtual_properties` | Arbitrary table properties specific to the target engine applied on SQLMesh virtual layer. Specified as key-value pairs (`key = value`) | dict | N |

| `physical_properties` | Arbitrary table properties specific to the target engine applied on SQLMesh's physical layer. Specified as key-value pairs (`key = value`)

| `virtual_properties` | Arbitrary table properties specific to the target engine applied on SQLMesh's virtual layer. Specified as key-value pairs (`key = value`) | dict | N |

georgesittas · 2024-05-04T11:21:27Z

sqlmesh/core/model/meta.py

Nit: I think this is expected to always exist right? Given that name is required.

Suggested change

model_name = values.get("name")

model_name = values["name"]

Yes, it feels like it should always exists as it's explicitly added in the provided validation fields. I'll add that change so that it fails eagerly if that behavior changes (thus preventing a silent switch to a Python model "None"' log).

georgesittas · 2024-05-04T11:27:53Z

sqlmesh/core/model/meta.py

Suggested change

physical_properties = {}

for expression in self.physical_properties_.expressions:

physical_properties[expression.this.name] = expression.expression

return physical_properties

return {e.this.name: e.expression for e in self.physical_properties_.expressions}

Thanks, those suggestions appears more "Pythonic" 👍

georgesittas · 2024-05-04T11:28:18Z

sqlmesh/core/model/meta.py

Suggested change

virtual_properties = {}

for expression in self.virtual_properties_.expressions:

virtual_properties[expression.this.name] = expression.expression

return virtual_properties

return {e.this.name: e.expression for e in self.virtual_properties_.expressions}

izeigerman · 2024-05-06T16:53:50Z

sqlmesh/core/engine_adapter/base.py

Model properties means something else throughout the code so let's not use it here. Additionally, the engine adapter is too low level of a construct to be aware of something as high-level as "model".

I'm ok keeping with keeping this as table_properties or just properties since that's what we create here. If you want to be more specific I'm also fine with something like table_or_view_properties.

Ok I see what you mean!

The goal of that change was to be consistent with the call parameters since we have both:

https://github.com/Kayrnt/sqlmesh/blob/8bae4b47b7bd5a038f05bd3d51a126c4c747fcf8/sqlmesh/core/engine_adapter/bigquery.py#L532

https://github.com/Kayrnt/sqlmesh/blob/8bae4b47b7bd5a038f05bd3d51a126c4c747fcf8/sqlmesh/core/engine_adapter/bigquery.py#L571

I would also suggest for table_or_view_properties (and table_or_view_properties_to_expressions) then as I'd rather have future maintainers understand that the function expects one or the other (and it's not a bug).

I'll make an update

izeigerman · 2024-05-06T16:55:49Z

sqlmesh/core/model/meta.py

This looks great 👍

izeigerman · 2024-05-06T17:04:42Z

Unfortunately, the migration script is required to rename table_properties to physical_properties in the existing SQLMesh state. One minor naming comment. Looks great otherwise, thanks!

…properties

Kayrnt · 2024-05-06T18:30:37Z

@izeigerman Looking a bit at the migration system, what you suggest is to create a new migration (something like v0050_table_properties_to_physical_properties) where:

I parse snapshot from snapshots table
I substitute the table_properties with an equivalent physical_properties
I save again updated snapshot from snapshots table

I could do it in SQL but I wonder if I can safely assume that all state connections can support JSON modifications or else try a replace on "table_properties" but then it could conflict with existing values. Any preference? I think the most simpler/safer would be to parse the JSON in Python but hopefully there's no installation with millions of rows to migrate else it might take ages to migrate.

izeigerman · 2024-05-06T20:14:36Z

@Kayrnt yes, the steps look accurate to me.

I think the most simpler/safer would be to parse the JSON in Python but hopefully there's no installation with millions of rows to migrate else it might take ages to migrate.

Yes, that's the safest approach which works uniformly across all supported engines. You shouldn't worry about millions of rows. It's going to be 10s of thousands at most.

izeigerman

This looks great, thanks a lot for addressing changes!

tobymao reviewed May 3, 2024

View reviewed changes

sqlmesh/core/model/common.py Outdated Show resolved Hide resolved

tobymao reviewed May 3, 2024

View reviewed changes

Kayrnt force-pushed the virtual_properties branch 2 times, most recently from 7506512 to 32bdcbd Compare May 3, 2024 18:39

Kayrnt marked this pull request as ready for review May 3, 2024 19:15

Kayrnt force-pushed the virtual_properties branch from 2fb4149 to 1632b98 Compare May 3, 2024 21:44

tobymao reviewed May 3, 2024

View reviewed changes

tobymao reviewed May 4, 2024

View reviewed changes

tests/core/test_model.py Outdated

Copy link
Copy Markdown

Contributor

tobymao May 4, 2024

Choose a reason for hiding this comment

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

ditto

tobymao reviewed May 4, 2024

View reviewed changes

tests/core/test_model.py Outdated

Copy link
Copy Markdown

Contributor

tobymao May 4, 2024

Choose a reason for hiding this comment

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

ditto

tobymao reviewed May 4, 2024

View reviewed changes

georgesittas changed the title ~~WIP - Add virtual properties and rename table_properties to physical_…~~ Feat: Add virtual properties and rename table_properties to physical_properties May 4, 2024

georgesittas reviewed May 4, 2024

View reviewed changes

Kayrnt force-pushed the virtual_properties branch 3 times, most recently from 4416d09 to 8bae4b4 Compare May 4, 2024 23:20

izeigerman reviewed May 6, 2024

View reviewed changes

sqlmesh/core/model/meta.py Outdated

Copy link
Copy Markdown

Collaborator

izeigerman May 6, 2024

Choose a reason for hiding this comment

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

This looks great 👍

Kayrnt reacted with hooray emoji

Kayrnt added 8 commits May 6, 2024 20:04

WIP - Add virtual properties and rename table_properties to physical_…

2dc62af

…properties

review changes

e120ff2

clean up tests

6776e34

review changes

2642fb8

linter & formatter fixes

00b1a4f

add missing placeholder in mock calls

fe936f4

review changes (revert engine related changes)

4d3f299

review changes: builder api for tests

8685c8a

Kayrnt added 2 commits May 6, 2024 20:04

Review changes: sql builder, doc & python refactor

7475e0f

review changes: renaming

c456af3

Kayrnt force-pushed the virtual_properties branch from 8bae4b4 to c456af3 Compare May 6, 2024 18:04

review changes: revert on snapshot change

9e8835f

izeigerman approved these changes May 7, 2024

View reviewed changes

izeigerman merged commit 0b9e184 into SQLMesh:main May 7, 2024

	- A key-value of arbitrary table properties specific to the target engine applied on SQLMesh physical layer. For example:
	- A key-value mapping of arbitrary table properties specific to the target engine applied on SQLMesh's physical layer. For example:

-| `physical_properties` | Arbitrary table properties specific to the target engine applied on SQLMesh physical layer. Specified as key-value pairs (`key = value`)
-| `virtual_properties` | Arbitrary table properties specific to the target engine applied on SQLMesh virtual layer. Specified as key-value pairs (`key = value`)                                                                                         |       dict        |    N     |
+| `physical_properties` | Arbitrary table properties specific to the target engine applied on SQLMesh's physical layer. Specified as key-value pairs (`key = value`)
+| `virtual_properties` | Arbitrary table properties specific to the target engine applied on SQLMesh's virtual layer. Specified as key-value pairs (`key = value`)                                                                                         |       dict        |    N     |

-            physical_properties = {}
-            for expression in self.physical_properties_.expressions:
-                physical_properties[expression.this.name] = expression.expression
-            return physical_properties
+            return {e.this.name: e.expression for e in self.physical_properties_.expressions}

-            virtual_properties = {}
-            for expression in self.virtual_properties_.expressions:
-                virtual_properties[expression.this.name] = expression.expression
-            return virtual_properties
+            return {e.this.name: e.expression for e in self.virtual_properties_.expressions}

Conversation

Kayrnt commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kayrnt May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

izeigerman May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

izeigerman May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kayrnt May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobymao commented May 3, 2024

Uh oh!

Kayrnt commented May 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobymao commented May 4, 2024

Uh oh!

georgesittas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kayrnt commented May 3, 2024 •

edited

Loading

Kayrnt May 3, 2024 •

edited

Loading

izeigerman May 3, 2024 •

edited

Loading

izeigerman May 3, 2024 •

edited

Loading

Kayrnt May 3, 2024 •

edited

Loading