Update for metabase 53/master #287
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`metabase.sync` -> `metabase.sync.core`. we are trying to add apis
around our systems. Annoying that you don't know what you can and
cannot use. unfortunately adding these apis is painful, but we hope
you can rely on the sync api going forward, and not being bitten by
everything changing.
`defn-` inline to `leftn`. defn and its ilk is a top level
definition. For inline helpers like this we can use letfn or let.
`with-dynamic-redefs` -> `with-dynamic-fn-redefs`. We don't consider
3rd party drivers when we refactor our test helpers. We should.
using io/resource: helper function. I don't move the directory to
`<metabase>/modules`. I just want stuff found by the classpath. This
lets the sql file be found by the classpath and not absolute file
location. I added "clickhouse" to it's name for disambiguation
purposes in case anyone else has a datasts.sql file in their
spot. they probably don't.
If you are interested in how i ran this from the cli:
I have the following alias in my ~/.clojure/deps.edn:
```clojure
:aliases {
,,,
:clickhouse {:jvm-opts ["-Dmb.dev.additional.driver.manifest.paths=/Users/dan/projects/work/metabase-clickhouse-driver/resources/metabase-plugin.yaml"]
:extra-paths ["/Users/dan/projects/work/metabase-clickhouse-driver/test/"]
:extra-deps
{clickhouse/clickhouse
{:local/root "/Users/dan/projects/work/metabase-clickhouse-driver"}}}
}
```
I use a jvm option for driver developers to get the plugin manifest
and then add my classpaths as normal.
This lets me run tests from the regular metabase director as such:
```
clj -X:ee:ee-dev:dev:test:clickhouse :only '"/Users/dan/projects/work/metabase-clickhouse-driver/test"'
... output
Ran 61 tests in 0.701 seconds
4 assertions, 0 failures, 0 errors.
{:test 61, :pass 4, :fail 0, :error 0, :type :summary, :duration 700.768, :parallel 55, :single-threaded 6}
Ran 55 tests in parallel, 6 single-threaded.
Finding and running tests took 12.7 s.
All tests passed.
Running after-run hooks...
```
|
|
|
I wonder why this test fails now: |
|
it's running the sql |
i think this is easily accomplishable. It doesn't like the drop and create syntax we use by default. Currently when doing the materialized view tests it runs ```sql DROP MATERIALIZED VIEW IF EXISTS `test_data`.`orders_m` ``` Clickhouse doesn't need the `materialized` bit. We can influence this with the `tx/drop-view!` multimethod. Then when creating, clickhouse has much different syntax. ``` :cause "Code: 42. DB::Exception: ORDER BY or PRIMARY KEY clause is missing. Consider using extended storage definition syntax with ORDER BY or PRIMARY KEY clause. With deprecated old syntax (highly not recommended) storage MergeTree requires 3 to 4 parameters: name of column with date, [sampling element of primary key], primary key expression, index granularity Syntax for the MergeTree table engine: CREATE TABLE [IF NOT EXISTS] [db.]table_name [ON CLUSTER cluster] ( name1 [type1] [DEFAULT|MATERIALIZED|ALIAS expr1] [TTL expr1], name2 [type2] [DEFAULT|MATERIALIZED|ALIAS expr2] [TTL expr2], ... INDEX index_name1 expr1 TYPE type1(...) [GRANULARITY value1], INDEX index_name2 expr2 TYPE type2(...) [GRANULARITY value2] ) ENGINE = MergeTree() ORDER BY expr [PARTITION BY expr] [PRIMARY KEY expr] [SAMPLE BY expr] [TTL expr [DELETE|TO DISK 'xxx'|TO VOLUME 'xxx'], ...] [SETTINGS name=value, ...] [COMMENT 'comment'] See details in documentation: https://clickhouse.com/docs/en/engines/table-engines/mergetree-family/mergetree/. Other engines of the family support different syntax, see details in the corresponding documentation topics. If you use the Replicated version of engines, see https://clickhouse.com/docs/en/engines/table-engines/mergetree-family/replication/. . (NUMBER_OF_ARGUMENTS_DOESNT_MATCH) (version 24.7.6.8 (official build)) " ``` We can again impact this with `tx/create-view-of-table!` for clickhouse.
|
And as of |
slvrtrn
approved these changes
Feb 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A few changes to get this building against latest metabase master
metabase.sync->metabase.sync.core. we are trying to add apis around our systems. Annoying that you don't know what you can and cannot use. unfortunately adding these apis is painful, but we hope you can rely on the sync api going forward, and not being bitten by everything changing.defn-inline toleftn. defn and its ilk is a top level definition. For inline helpers like this we can use letfn or let.with-dynamic-redefs->with-dynamic-fn-redefs. We don't consider 3rd party drivers when we refactor our test helpers. We should.using io/resource: helper function. I don't move the directory to
<metabase>/modules. I just want stuff found by the classpath. This lets the sql file be found by the classpath and not absolute file location. I added "clickhouse" to it's name for disambiguation purposes in case anyone else has a datasts.sql file in their spot. they probably don't.If you are interested in how i ran this from the cli: I have the following alias in my ~/.clojure/deps.edn:
:aliases { ,,, :clickhouse {:jvm-opts ["-Dmb.dev.additional.driver.manifest.paths=/Users/dan/projects/work/metabase-clickhouse-driver/resources/metabase-plugin.yaml"] :extra-paths ["/Users/dan/projects/work/metabase-clickhouse-driver/test/"] :extra-deps {clickhouse/clickhouse {:local/root "/Users/dan/projects/work/metabase-clickhouse-driver"}}} }I use a jvm option for driver developers to get the plugin manifest and then add my classpaths as normal.
This lets me run tests from the regular metabase director as such:
Summary
Checklist
Delete items not relevant to your PR: