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

Bug/223 relationship test with limit #245

Merged
merged 5 commits into from
Feb 4, 2024
Merged

Bug/223 relationship test with limit #245

merged 5 commits into from
Feb 4, 2024
+49 −2

Conversation

BentsiLeviav
Copy link
Contributor

Summary

As mentioned in issue #223, dbt fails to execute relationship tests when a limit clause is specified. The issue arises because the relationship test macro appends the join_use_nulls settings, and the limit is added at the end of the query (after the settings), causing a parsing error.

To address this limitation of adding a limit clause when the query includes settings, we developed a new macro that parses the query and inserts the limit just before the settings clause.

This is accomplished by introducing a flag within a comment, which is then used to parse the settings section accordingly.

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG

@BentsiLeviav BentsiLeviav requested a review from genzgd January 28, 2024 18:42
@genzgd
Copy link
Contributor

genzgd commented Feb 1, 2024

Maybe we could always add an -- end_of_sql marker (regardless of whether there are settings), instead of a -- settings_section marker only if there are settings. That would allow finding the last "-- end_of_sql" marker and insert the limit there, regardless of any nesting that may happen in building the query.

Since there aren't any real automated dbt tests around the dbt test command, has this been validated manually?

@BentsiLeviav
Copy link
Contributor Author

@genzgd Yes, -- end_of_sql is better. I changed the code.
I ran the tests, and everything seems to be OK besides 8 tests (which were broken even before my changes).

genzgd
genzgd requested changes Feb 4, 2024
View reviewed changes
dbt/adapters/clickhouse/impl.py
@@ -412,15 +412,30 @@ def get_model_settings(self, model):
res = []
for key in settings:
res.append(f' {key}={settings[key]}')
return '' if len(res) == 0 else 'SETTINGS ' + ', '.join(res) + '\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's always add the -- end_of_sql marker comment regardless of whether we have actual settings to add? That way we can check for it in other circumstances where we need to do stuff after the "main" sql.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to add that at the end of every macro?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be the same macros where we add settings now, just always add -- end_of_sql instead of conditionally.

I think the settings thing is fairly unique to ClickHouse so I feel like being able to consistently determine "this is the end of regular SQL" from "this might be ClickHouse doing nonstandard stuff" will lead to more straightforward code, even if we don't actually add the settings. Note that there are so many possible query and table settings that we'll probably end up with them most of the time anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened a new PR in #249

@genzgd genzgd merged commit d8afb93 into main Feb 4, 2024
32 checks passed
genzgd added a commit that referenced this pull request Feb 4, 2024
@genzgd
Revert "Bug/223 relationship test with limit (#245)"
54df20c 
This reverts commit d8afb93.
@genzgd genzgd mentioned this pull request Feb 4, 2024
Merged
genzgd added a commit that referenced this pull request Feb 4, 2024
@genzgd
Revert "Bug/223 relationship test with limit (#245)" (#247)
b791bcc 
This reverts commit d8afb93.
@BentsiLeviav BentsiLeviav mentioned this pull request Feb 13, 2024
Merged
Solovechik added a commit to tekliner/dbt-clickhouse that referenced this pull request May 27, 2024
@Solovechik @Schum-io
@genzgd @PrVrSs @gladkikhtutu @damirbk @zli06160 @the4thamigo-uk @gfunc @kris947 @StevenReitsma @SoryRawyer @ptemarvelde @dstsimokha @BentsiLeviav @silentsokolov @dependabot @tevariou @dwreeves @Ian2012 @Somtom @scrawfor @rjoelnorgren
Fork update (#10)
004a4f0 
* fix: SYSTEM SYNC REPLICA for on_cluster_clause (ClickHouse#156)

* fix SYSTEM SYNC REPLICA

* add schema

* Update version and pypi job

* Fix incompatible return type (ClickHouse#162)

* Distributed table materialization (ClickHouse#163)

* distributed table materialization

* fix rebase

* PR fixes

* Bump version

* Tweak PyPI build Python release

* Add space to exchange_tables_atomic macro (ClickHouse#168)

* Add space to exchange_tables_atomic macro

This changes the SYSTEM SYNC REPLICA query to have a space between the
ON CLUSTER clause and the table name.

* Move whitespace to on_cluster_clause

* Fix bad logging/error handling (ClickHouse#170)

* Distributed incremental materialization (ClickHouse#172)

* distributed table materialization

* fix rebase

* PR fixes

* distributed incremental materialization

* fix

* fix

* add insert_distributed_sync to README.md

* add checks on  insert_distributed_sync

* add checks on  insert_distributed_sync

* review fixes

* Update version and tweak docs

* Lw delete set fix (ClickHouse#174)

* Move lightweight delete settings to per query for HTTP stickiness fix

* Minor cleanup and doc updates

* Fix legacy incremental materialization (ClickHouse#178)

* fix: distributed_table materialization issue (ClickHouse#184)

* Bump version and changelog (ClickHouse#185)

* cluster names containing dash characters (ClickHouse#198) (ClickHouse#200)

Co-authored-by: the4thamigo-uk <the4thamigo-uk>

* Add basic error test, fix minor merge conflict (ClickHouse#202)

* Cluster setting and Distributed Table tests (ClickHouse#186)

* added can_on_cluster var in ClickhouseRelation

* add tests for cluster

* fix lint issue

* debug set cluster env variable

* debug test

* debug and add tests

* skip distributed table grant test

* debug workflow

* debug workflow

* debug test

* add tests fro distributed_incremental

* fix zk path error

* fix wrong alias for distributed materializations

update aliase test

* update base on review

* Update version and CHANGELOG, incorporate cluster name fix (ClickHouse#203)

* Release 1 5 0 (ClickHouse#210)

* Initial 1.5.0 commit

* Reorganize basic tests

* Fix lint

* Add case sensitive cache

* Fix s3 bucket bug

* Checkpoint for constraints/contracts

* Fix native column query

* Loosen replication test

* Checkpoint for constraints tests

* Checkpoint for constraints tests

* Add rendering of model level CHECK constraints

* Fix lint

* Reorganize test files

* Add one hooks test

* Fix lint

* Update test and dependency versions. (ClickHouse#211)

* Adjust the wrapper parenthesis around the table materialization sql code (ClickHouse#212)

* Update for 1.5.1 bug fix

* Fix creation of replicated tables when using legacy materialization (ClickHouse#208)

* On cluster sync cleanup

* Bug fixes related to model settings. (ClickHouse#214)

* Add materialization macro for materialized view (ClickHouse#207)

* Add materialization macro for materialized view

* fix isort issues in materialized view test

* Release 1 6 0 (ClickHouse#215)

* Initial dbt 1.6 update

* Add skipped clone test

* Clean up MV PR

* Release 1 6 1 (ClickHouse#217)

* Identifier quoting checkpoint

* Identifier quoting checkpoint

* Fix distributed table local quoting

* Fix issues with deduplication settings

* Release 1 6 2 (ClickHouse#219)

* Limited fix to completely broken `on_schema_change`

* Tweak changelog

* Release 1 7 0 (ClickHouse#220)

* Initial dependency updates for 1.7.x

* Initial dependency updates for 1.7.x

* Correctly warn or error if light weight deletes not available

* Wrap columns_in_query query in select statement (ClickHouse#222)

* Wrap columns_in_query query in select statement

* formatting

* Update changelog

* allows to add a comment in table's or view's metadata

* add settings_section flag as comment for code using settings

* override test sql macro and add limit-placer macro

* update CHANGELOG.md

* fix: use correct schema for MV target tables (ClickHouse#244)

* fix: use correct schema when updating MVs

The existing implementation passes just the name for `target_table`,
which ultimately means that the target schema is not included when the
final SQL is generated. By passing the entire relation object, the
correct target schema will be present in the final SQL.

* update MV tests

Provide a custom schema to make sure that the full target table
name (schema + relation name) is included in the CREATE MATERIALIZED
VIEW statement

* Update changelog

* rename end of query flag

* Bug/223 relationship test with limit (ClickHouse#245)

* add settings_section flag as comment for code using settings

* override test sql macro and add limit-placer macro

* update CHANGELOG.md

* rename end of query flag

* Revert "Bug/223 relationship test with limit (ClickHouse#245)" (ClickHouse#247)

This reverts commit d8afb93.

* always return --end_of_sql when asking for settings

* Add model settings based on materialization type

* support setting clause on view creation

* edit CHANGELOG.md

* Bump version and tweak changelog

* change list syntax to satisfy lint test

* change list syntax to satisfy lint test

* change imports order to satisfy lint test

* Add typing to satisfy lint

* Add snapshot materialization to default settings

* Fix tests - add distributed_table and distributed_incremental materializations

* Fix tests - make sure to call the get_model_settings only when materialization is view

* clean up recent changelog

* Add materialization macro for dictionaries

* address lint issue in dictionary test

* address lint issue with enum

* Fix model settings with custom materialization

* Release 1.7.4 housekeeping (ClickHouse#261)

* Bump black from 23.11.0 to 24.3.0 (ClickHouse#259)

Bumps [black](https://github.com/psf/black) from 23.11.0 to 24.3.0.
- [Release notes](https://github.com/psf/black/releases)
- [Changelog](https://github.com/psf/black/blob/main/CHANGES.md)
- [Commits](psf/black@23.11.0...24.3.0)

---
updated-dependencies:
- dependency-name: black
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Release 1 7 5 (ClickHouse#265)

* Release 1.7.5 housekeeping

* Upgrade setuptools requirement for clickhouse_driver install

* Remove flake8 checks for the moment

* Update workflow actions

* Fix black comma

* fix(clients): add newlines around subquery when retrieving columns to avoid a syntax error (ClickHouse#262)

* Fix lint

* lazy load agate (ClickHouse#263)

* feat: add TTL support (ClickHouse#254)

* Fix lint

* Update table relation after exchange command (ClickHouse#230)

Related to ClickHouse#226

* feat: allow to add connection overrides for dictionaries (ClickHouse#267)

* Housekeeping for 1.7.6 release (ClickHouse#268)

* Revert "allows to add a comment in table's or view's metadata"

* Fix bool_or behavior (ClickHouse#270)

* feat: support column codecs

* Use Column.data_type in ClickHouseAdapter.format_columns

* Always apply query_settings in clickhouse__insert_into macro

* Add ClickHouseColumn.is_low_cardinality

* Update column type test cases for LowCardinality

* Omit empty dictionary connection_overrides from materialization DDL

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Schum <68906108+Schum-io@users.noreply.github.com>
Co-authored-by: Geoff Genz <geoff@clickhouse.com>
Co-authored-by: Sergey Reshetnikov <resh.sersh@gmail.com>
Co-authored-by: gladkikhtutu <88535677+gladkikhtutu@users.noreply.github.com>
Co-authored-by: Damir Basic Knezevic <damirbasicknezevic@gmail.com>
Co-authored-by: Zhenbang <122523068+zli06160@users.noreply.github.com>
Co-authored-by: Andy <email@elevatesystems.co.uk>
Co-authored-by: gfunc <fcjchaojian@gmail.com>
Co-authored-by: Kristof Szaloki <szalokikristof@gmail.com>
Co-authored-by: Steven Reitsma <4895139+StevenReitsma@users.noreply.github.com>
Co-authored-by: Rory Sawyer <rory@sawyer.dev>
Co-authored-by: ptemarvelde <45282601+ptemarvelde@users.noreply.github.com>
Co-authored-by: Dmitrii Tcimokha <dstsimokha@gmail.com>
Co-authored-by: bentsileviav <bentsi.leviav@clickhouse.com>
Co-authored-by: Dmitriy Sokolov <silentsokolov@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: triou <teva.riou@gmail.com>
Co-authored-by: Daniel Reeves <31971762+dwreeves@users.noreply.github.com>
Co-authored-by: Cristhian Garcia <crisgarta8@gmail.com>
Co-authored-by: Thomas Schmidt <somtom91@gmail.com>
Co-authored-by: scrawfor <scrawfor@users.noreply.github.com>
Co-authored-by: Robin Norgren <68205730+rjoelnorgren@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@genzgd genzgd genzgd requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BentsiLeviav @genzgd