Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove TODO expr in filters #70

Closed
wants to merge 1 commit into from
Closed

Remove TODO expr in filters #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion semantic_model_generator/generate_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ def _get_placeholder_filter() -> List[semantic_model_pb2.NamedFilter]:
name=_PLACEHOLDER_COMMENT,
synonyms=[_PLACEHOLDER_COMMENT],
description=_PLACEHOLDER_COMMENT,
expr=_PLACEHOLDER_COMMENT,
)
]

Expand Down
8 changes: 4 additions & 4 deletions semantic_model_generator/tests/generate_model_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ def mock_dependencies_exceed_context(mock_snowflake_connection):
def test_raw_schema_to_semantic_context(
mock_dependencies, mock_snowflake_connection, mock_snowflake_connection_env
):
want_yaml = "name: this is the best semantic model ever\ntables:\n - name: ALIAS\n description: some table comment\n base_table:\n database: TEST_DB\n schema: SCHEMA_TEST\n table: ALIAS\n filters:\n - name: ' '\n synonyms:\n - ' '\n description: ' '\n expr: ' '\n dimensions:\n - name: ZIP_CODE\n synonyms:\n - ' '\n description: some column comment\n expr: ZIP_CODE\n data_type: TEXT\n time_dimensions:\n - name: BAD_ALIAS\n synonyms:\n - ' '\n description: ' '\n expr: BAD_ALIAS\n data_type: TIMESTAMP\n measures:\n - name: AREA_CODE\n synonyms:\n - ' '\n description: ' '\n expr: AREA_CODE\n data_type: NUMBER\n - name: CBSA\n synonyms:\n - ' '\n description: ' '\n expr: CBSA\n data_type: NUMBER\n"
want_yaml = "name: this is the best semantic model ever\ntables:\n - name: ALIAS\n description: some table comment\n base_table:\n database: TEST_DB\n schema: SCHEMA_TEST\n table: ALIAS\n filters:\n - name: ' '\n synonyms:\n - ' '\n description: ' '\n dimensions:\n - name: ZIP_CODE\n synonyms:\n - ' '\n description: some column comment\n expr: ZIP_CODE\n data_type: TEXT\n time_dimensions:\n - name: BAD_ALIAS\n synonyms:\n - ' '\n description: ' '\n expr: BAD_ALIAS\n data_type: TIMESTAMP\n measures:\n - name: AREA_CODE\n synonyms:\n - ' '\n description: ' '\n expr: AREA_CODE\n data_type: NUMBER\n - name: CBSA\n synonyms:\n - ' '\n description: ' '\n expr: CBSA\n data_type: NUMBER\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, the expr is also auto-populated for filters.
I thought this model'll still be broken without expr for filters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. Looking

Copy link
Collaborator

Choose a reason for hiding this comment

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

one thought maybe we mark the whole filter area as comments.... and let user know you can uncomment if you want to add filter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought this was the issue, but turns out this is a valid filter.

    filters:
      - name: '  ' # <FILL-OUT>
        synonyms:
          - '  ' # <FILL-OUT>
        description: '  ' # <FILL-OUT>
        expr: '  ' # <FILL-OUT>

And we already include the col name in the expr (so we are generating valid yaml in this generator).


snowflake_account = "test_account"
base_tables = ["test_db.schema_test.ALIAS"]
Expand Down Expand Up @@ -365,7 +365,7 @@ def test_generate_base_context_with_placeholder_comments(
mock_file.assert_called_once_with(output_path, "w")
# Assert file save called with placeholder comments added.
mock_file().write.assert_called_once_with(
"name: my awesome semantic model\ntables:\n - name: ALIAS\n description: some table comment\n base_table:\n database: TEST_DB\n schema: SCHEMA_TEST\n table: ALIAS\n filters:\n - name: ' ' # <FILL-OUT>\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: ' ' # <FILL-OUT>\n dimensions:\n - name: ZIP_CODE\n synonyms:\n - ' ' # <FILL-OUT>\n description: some column comment\n expr: ZIP_CODE\n data_type: TEXT\n time_dimensions:\n - name: BAD_ALIAS\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: BAD_ALIAS\n data_type: TIMESTAMP\n measures:\n - name: AREA_CODE\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: AREA_CODE\n data_type: NUMBER\n - name: CBSA\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: CBSA\n data_type: NUMBER\n"
"name: my awesome semantic model\ntables:\n - name: ALIAS\n description: some table comment\n base_table:\n database: TEST_DB\n schema: SCHEMA_TEST\n table: ALIAS\n filters:\n - name: ' ' # <FILL-OUT>\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n dimensions:\n - name: ZIP_CODE\n synonyms:\n - ' ' # <FILL-OUT>\n description: some column comment\n expr: ZIP_CODE\n data_type: TEXT\n time_dimensions:\n - name: BAD_ALIAS\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: BAD_ALIAS\n data_type: TIMESTAMP\n measures:\n - name: AREA_CODE\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: AREA_CODE\n data_type: NUMBER\n - name: CBSA\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: CBSA\n data_type: NUMBER\n"
)


Expand Down Expand Up @@ -395,7 +395,7 @@ def test_generate_base_context_with_placeholder_comments_cross_database_cross_sc

# Assert file save called with placeholder comments added along with sample values and cross-database
mock_file().write.assert_called_once_with(
"name: Another Incredible Semantic Model\ntables:\n - name: ALIAS\n description: some table comment\n base_table:\n database: TEST_DB\n schema: SCHEMA_TEST\n table: ALIAS\n filters:\n - name: ' ' # <FILL-OUT>\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: ' ' # <FILL-OUT>\n dimensions:\n - name: ZIP_CODE\n synonyms:\n - ' ' # <FILL-OUT>\n description: some column comment\n expr: ZIP_CODE\n data_type: TEXT\n time_dimensions:\n - name: BAD_ALIAS\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: BAD_ALIAS\n data_type: TIMESTAMP\n measures:\n - name: AREA_CODE\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: AREA_CODE\n data_type: NUMBER\n - name: CBSA\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: CBSA\n data_type: NUMBER\n - name: PRODUCTS\n description: ' ' # <FILL-OUT>\n base_table:\n database: A_DIFFERENT_DATABASE\n schema: A_DIFFERENT_SCHEMA\n table: PRODUCTS\n filters:\n - name: ' ' # <FILL-OUT>\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: ' ' # <FILL-OUT>\n measures:\n - name: SKU\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: SKU\n data_type: NUMBER\n sample_values:\n - '1'\n - '2'\n - '3'\n"
"name: Another Incredible Semantic Model\ntables:\n - name: ALIAS\n description: some table comment\n base_table:\n database: TEST_DB\n schema: SCHEMA_TEST\n table: ALIAS\n filters:\n - name: ' ' # <FILL-OUT>\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n dimensions:\n - name: ZIP_CODE\n synonyms:\n - ' ' # <FILL-OUT>\n description: some column comment\n expr: ZIP_CODE\n data_type: TEXT\n time_dimensions:\n - name: BAD_ALIAS\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: BAD_ALIAS\n data_type: TIMESTAMP\n measures:\n - name: AREA_CODE\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: AREA_CODE\n data_type: NUMBER\n - name: CBSA\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: CBSA\n data_type: NUMBER\n - name: PRODUCTS\n description: ' ' # <FILL-OUT>\n base_table:\n database: A_DIFFERENT_DATABASE\n schema: A_DIFFERENT_SCHEMA\n table: PRODUCTS\n filters:\n - name: ' ' # <FILL-OUT>\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n measures:\n - name: SKU\n synonyms:\n - ' ' # <FILL-OUT>\n description: ' ' # <FILL-OUT>\n expr: SKU\n data_type: NUMBER\n sample_values:\n - '1'\n - '2'\n - '3'\n"
)


Expand Down Expand Up @@ -494,7 +494,7 @@ def test_generate_base_context_from_table_that_has_too_long_context(

mock_file.assert_called_once_with(output_path, "w")
mock_logger.warning.assert_called_once_with(
"WARNING 🚨: The Semantic model is too large. \n Passed size is 26867 characters. We need you to remove 784 characters in your semantic model. Please check: \n (1) If you have long descriptions that can be truncated. \n (2) If you can remove some columns that are not used within your tables. \n (3) If you have extra tables you do not need. \n (4) If you can remove sample values. \n Once you've finished updating, please validate your semantic model."
"WARNING 🚨: The Semantic model is too large. \n Passed size is 26848 characters. We need you to remove 768 characters in your semantic model. Please check: \n (1) If you have long descriptions that can be truncated. \n (2) If you can remove some columns that are not used within your tables. \n (3) If you have extra tables you do not need. \n (4) If you can remove sample values. \n Once you've finished updating, please validate your semantic model."
)

mock_file.assert_called_once_with(output_path, "w")
Expand Down
Loading