Skip to content

Chore: update docs to include MacroEvaluator.columns_to_types#1644

Merged
georgesittas merged 18 commits intomainfrom
jo/document_macro_evaluator_columns_to_types
Nov 7, 2023
Merged

Chore: update docs to include MacroEvaluator.columns_to_types#1644
georgesittas merged 18 commits intomainfrom
jo/document_macro_evaluator_columns_to_types

Conversation

@georgesittas
Copy link
Contributor

@georgesittas georgesittas commented Nov 1, 2023

cc @z3z1ma

@georgesittas georgesittas requested a review from treysp November 1, 2023 02:46
@z3z1ma
Copy link
Contributor

z3z1ma commented Nov 1, 2023

Nice 2 questions.

  1. Where does this not work. You mention "where columns can be statically determined". Is there any way we can succinctly enumerate a case or two. I have only used the feature on external models thus far. Does it work on other sqlmesh models?

  2. Is this there preferred way to get the correct key for the columns_to_types dict? I could see that being a point of contention if it is not clear as you could end up trying to access model_name.this, model_name.key, model_name.alias or who knows. Its obvious enough once you use the lib, but emphasizing the access pattern would be good.

evaluator.columns_to_types(model_name.sql())

cc @georgesittas

@georgesittas
Copy link
Contributor Author

Thank you for reviewing, @z3z1ma @treysp! Agree with most of the points you've made and will update this PR soon.

@georgesittas
Copy link
Contributor Author

Nice 2 questions.

  1. Where does this not work. You mention "where columns can be statically determined". Is there any way we can succinctly enumerate a case or two. I have only used the feature on external models thus far. Does it work on other sqlmesh models?

  2. Is this there preferred way to get the correct key for the columns_to_types dict? I could see that being a point of contention if it is not clear as you could end up trying to access model_name.this, model_name.key, model_name.alias or who knows. Its obvious enough once you use the lib, but emphasizing the access pattern would be good.

evaluator.columns_to_types(model_name.sql())

cc @georgesittas

@z3z1ma regarding (1), IIRC it doesn't work when SQLMesh doesn't know or can't infer the schema of a model. For example, if you don't run create_external_models, then it's supposed to fail if columns_to_types is called with an external model's name for which we don't know the schema.

(2) is probably trickier because it depends on the use-case. If for example a macro gets passed a "foo.bar" then you don't want to generate that arg using .sql() because it'll include the quotes which need to be omitted for the lookup.

@georgesittas georgesittas force-pushed the jo/document_macro_evaluator_columns_to_types branch from 6bf78e1 to f85e5ea Compare November 7, 2023 00:51
@georgesittas georgesittas requested review from a team and treysp November 7, 2023 01:33
@georgesittas georgesittas merged commit a98af5b into main Nov 7, 2023
@georgesittas georgesittas deleted the jo/document_macro_evaluator_columns_to_types branch November 7, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants