Skip to content

Fix: Add adapters for commands that are not using the snapshot evaluator#3531

Merged
themisvaltinos merged 6 commits intomainfrom
themis/engine_adapters_fix
Jan 3, 2025
Merged

Fix: Add adapters for commands that are not using the snapshot evaluator#3531
themisvaltinos merged 6 commits intomainfrom
themis/engine_adapters_fix

Conversation

@themisvaltinos
Copy link
Copy Markdown
Collaborator

@themisvaltinos themisvaltinos commented Dec 17, 2024

This fixes an omission on the latest multi-engine feature, to create the engine adapters in cases when they're not generated in the snapshot_evaluator property, ie for the commands render, table_diff and create_test

@themisvaltinos themisvaltinos requested review from a team and sungchun12 December 17, 2024 22:20
@erindru
Copy link
Copy Markdown
Collaborator

erindru commented Dec 17, 2024

No existing tests picked up this regression, are you able to add a test?

@themisvaltinos
Copy link
Copy Markdown
Collaborator Author

No existing tests picked up this regression, are you able to add a test?

Yes because these commands wouldnt work only for models which had the gateway specified property. I will think of tests to add as well

@themisvaltinos themisvaltinos force-pushed the themis/engine_adapters_fix branch from 5c501f2 to 01969a8 Compare December 18, 2024 11:47
@themisvaltinos themisvaltinos force-pushed the themis/engine_adapters_fix branch from d02ffcf to a31a67d Compare December 20, 2024 11:33
@themisvaltinos themisvaltinos force-pushed the themis/engine_adapters_fix branch from 95a012c to 07d048c Compare December 22, 2024 11:15
if config.model_defaults.dialect != ctx.dialect:
config.model_defaults = config.model_defaults.copy(update={"dialect": ctx.dialect})

# To enable parallelism in integration tests
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why does this impact parallelism?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

To isolate the connections per test because otherwise they will encounter concurrency issues, as they will end up using the same connections. Since all the engine adapters are created upfront and this particular file's tests are using for the context the same config: https://github.com/TobikoData/sqlmesh/blob/main/tests/core/engine_adapter/integration/config.yaml

@themisvaltinos themisvaltinos force-pushed the themis/engine_adapters_fix branch from 07d048c to 0c258c3 Compare January 2, 2025 19:37
@themisvaltinos themisvaltinos force-pushed the themis/engine_adapters_fix branch from 0c258c3 to e9bc257 Compare January 3, 2025 08:22
@themisvaltinos themisvaltinos merged commit e9efeff into main Jan 3, 2025
@themisvaltinos themisvaltinos deleted the themis/engine_adapters_fix branch January 3, 2025 10:14
treysp pushed a commit to mesmith027/sqlmesh that referenced this pull request Jan 7, 2025
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.

4 participants