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

Jump to bottom

Add Trino Support #944

Merged
merged 5 commits into from
Dec 19, 2023
Merged

Add Trino Support #944

merged 5 commits into from
Dec 19, 2023

Conversation

tlento
Copy link
Contributor

@tlento tlento commented Dec 19, 2023 •
edited
Loading

Resolves #207

This is a mechanical re-work of #810 , which was submitted by @sarbmeetka . For details on the Trino-related changes, please see @sarbmeetka 's excellent description on the original PR.

This PR is necessary because we needed to run all of our engine tests on the output, but we cannot access secrets in the environment from a PR initiated off of a fork from a community member. While this seems like something we should be able to adjust, the expedient path at this time is to simply re-work the PR separately here.

There are three primary differences between this PR and the original in #810

  1. This PR consolidates all of the original commits into a single update for ease of management with local and CI testing. To see the original sequence of fixes please refer to the original PR.
  2. This PR splits the machine-generated snapshots into a separate commit, again for ease of paging through the main set of changes, and also for ease of readability for future viewers of the main branch log. This comes at the expense of potentially adding a commit where tests are just broken, but sometimes you can't have everything, and merge commits exist for a reason.
  3. This PR adds a commit to put Trino testing into our CI/CD configuration, along with some associated pyproject.toml fixups.

Of these, the third change is the only thing that is new from the original PR.

sarbmeetka and others added 2 commits December 18, 2023 18:25
@sarbmeetka @tlento
Add Trino adapter support for MetricFlow
e3fd61f 
This commit, authored by @sarbmeetka, includes all of the material
changes relevant for adding support for Trino to MetricFlow.

Specifically, this includes the SqlQueryPlanRenderer for Trino and
integration in the CLI and unit test suite with the dbt-trino
adapter. All other changes - particularly those to the contents
of test queries - were made in order to bring our internal test
SQL in line with Trino's ANSI-compliance requirements.

The original PR (#810)
commit stack has been consolidated into this one, and a separate
commit with the auto-generated snapshot files for the Trino test
cases. This has been done to make isolating fixes a bit simpler
for CI testing prior to merge.
@tlento
Generate snapshots for Trino
d1fcc62 
This is a one-shot re-do of the snapshot generation for Trino
test cases originally done throughout @sarbmeetka's PR
(#810).

As explained earlier, these were split out to make it easier to
isolate the non-automated changes for any necessary updates in
CI testing.
@cla-bot cla-bot bot added the cla:yes label Dec 19, 2023
@tlento tlento added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Dec 19, 2023
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS December 19, 2023 02:46 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS December 19, 2023 02:46 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS December 19, 2023 02:46 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS December 19, 2023 02:46 — with GitHub Actions Inactive
@github-actions github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Dec 19, 2023
@tlento
Add Trino engine test config to SQL Engine Tests action
bb739f5 
This adds a Trino test job to the MetricFlow SQL Engine Tests
action.

Trino can run its tests in a service container without need for
an external environment, however, the service container execution
for these tests is materially slower in CI runs than Postgres.

Rather than delay PR-blocking unit test completion we put the Trino
test suite execution in the label-initiated SQL Engine test action.
This allows us to move Trino to cloud-hosted execution, if we should
care to do so, but for the time being we will stick with the in-memory
service container.
@tlento tlento added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Dec 19, 2023
@tlento tlento had a problem deploying to DW_INTEGRATION_TESTS December 19, 2023 02:57 — with GitHub Actions Failure
@tlento tlento had a problem deploying to DW_INTEGRATION_TESTS December 19, 2023 02:57 — with GitHub Actions Failure
@tlento tlento had a problem deploying to DW_INTEGRATION_TESTS December 19, 2023 02:57 — with GitHub Actions Failure
@tlento tlento had a problem deploying to DW_INTEGRATION_TESTS December 19, 2023 02:57 — with GitHub Actions Failure
@tlento
Make pyproject.toml indentation consistent
2ca9f48
@tlento
Add missing Trino dependencies to Trino env
7e931bf 
Somehow this got removed in the various PR manipulations, so
local tests were passing but CI (and fresh installs) will fail.
@tlento tlento added Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment and removed Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment labels Dec 19, 2023
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS December 19, 2023 03:06 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS December 19, 2023 03:06 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS December 19, 2023 03:06 — with GitHub Actions Inactive
@tlento tlento temporarily deployed to DW_INTEGRATION_TESTS December 19, 2023 03:06 — with GitHub Actions Inactive
@tlento tlento marked this pull request as ready for review December 19, 2023 03:16
@tlento tlento mentioned this pull request Dec 19, 2023
Closed
@github-actions github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Dec 19, 2023
@tlento tlento merged commit aa7724b into main Dec 19, 2023
25 checks passed
@tlento tlento deleted the add-trino-support-with-cd-config branch December 19, 2023 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcodamore marcodamore marcodamore approved these changes

@courtneyholcomb courtneyholcomb Awaiting requested review from courtneyholcomb

@plypaul plypaul Awaiting requested review from plypaul

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Trino
3 participants
@tlento @marcodamore @sarbmeetka