Add Trino Support #294
Add Trino Support #294
Conversation
Fix ORDER BY to order by columns which are exptected to have unique values across them, instead of random column
…tests row_conditions
|
@clausherther Could you approve the workflow in CircleCI, to verify if all tests will pass? |
|
Thanks for this! I'll review by end of week. |
| @@ -40,7 +40,7 @@ | |||
| with column_values as ( | |||
|
|
|||
| select | |||
| row_number() over(order by 1) as row_index, | |||
| row_number() over(order by {{ columns|join(', ') }}) as row_index, | |||
There was a problem hiding this comment.
Was this failing? Is this required for Trino support? I'd have to look at this separately to step through, it's been a while, but if this is a bug, I'd rather roll this into a separate PR.
There was a problem hiding this comment.
In Trino, ordering by column's position number inside ranking functions doesn't work. row_number() over(order by 1) results in a random order. This test fails because of that.
However, IMHO it is a bug anyway.
Mentioned test passes in other dbt adapters, because in current test model the first column idx contains subsequent id numbers. But, for example, we could have same value for all rows in the first column, and we will receive a random order.
IMHO ordering by all columns that are expected to have unique values makes more sense, than ordering by the first arbitrary column.
WDYT? I can create a separate PR for this change, if you agree with the above.
There was a problem hiding this comment.
Makes sense, thanks! Let me look at this some more. If this is a bug surfaced because of the Trino integration, it would make sense to leave it here. Just want to look at the data in this case to understand this better.
...schema_tests/aggregate_functions/expect_column_proportion_of_unique_values_to_be_between.sql
Outdated
Show resolved
Hide resolved
damian3031
changed the title
Co-authored-by: Damian Owsianny <[email protected]>
|
Hi @clausherther, any progress on reviewing this PR? |
Sorry, holidays and family stuff got in the way, I'll try to get to it this week! |
|
Thanks for all the hard work on this. I really appreciate it. |
|
Hi @clausherther, any update here? |
|
We are still interested in this as well. Thanks for the effort on this. |
Issue this PR Addresses/Closes
Closes #293
Summary of Changes
Reviewers
@clausherther