v.46

[Maiquu]

Notable Changes

Updated tests to use same catalog (c292b9a)

This commit is same as the changes made to presto-jdbc driver at metabase repo. Detected that few of the new tests tries to create a dynamically named database so this change might be mandatory for future. (I forgot where the mentioned test is located, I will edit here later linking the related file.)

Migrated to Honey SQL 2 (8c3a641)

Reference. Should close #88.

Updated datatime_diff implementation to use the new multimethod (f49d892)

From driver-changelog

The multimethod metabase.driver.sql.query-processor/datetime-diff has been added. This method is used by implementations of ->honeysql for the :datetime-diff clause. It is recommended to implement this if you want to use the default SQL implementation of ->honeysql for the :datetime-diff, which includes validation of argument types across all units.

Bumped metabase and clojure versions (8a45d0b)

Bumped clojure version according to prepare-backend.yml github action in metabase repository.

[Maiquu]

I think I know why it failed. I did not commit a specific portion of a file thinking it wouldnt be necessary. I will update soon.

[Maiquu]

The failed test will probably never succeed with trino/starburst at the moment.

Trino does not support ORDER BY in nested queries which causes this test to fail. They also excluded presto-jdbc from this specific test. Reason why this did not fail before is because they removed the :foreign-key constraint from the test which was preventing it from being run with starburst prior to v1.46.0.

I'm not really sure how to prevent that test from running.

SQL generated at test:

SELECT "source"."date" AS "date",
  "source"."sum" AS "sum",
  "source"."sum_2" AS "sum_2"
FROM (
    SELECT DATE_TRUNC('month', "default"."test_data_checkins"."date") AS "date",
      SUM("default"."test_data_checkins"."user_id") AS "sum",
      SUM("default"."test_data_checkins"."venue_id") AS "sum_2"
    FROM "default"."test_data_checkins"
    GROUP BY DATE_TRUNC('month', "default"."test_data_checkins"."date")
    ORDER BY DATE_TRUNC('month', "default"."test_data_checkins"."date") ASC
  ) AS "source"
WHERE "source"."sum" > 300
LIMIT 2

[andrewdibiasio6]

Looks like this takes care of: https://github.com/starburstdata/metabase-driver/issues/86

[andrewdibiasio6]

I'm not really sure how to prevent that test from running.

I believe we can override the test, and provide a "stubbed" execution with what we want, moving/removing the inner order by.

Here is an example of another driver that overrides a unit test text-equals-empty-string-test:

https://github.com/exasol/metabase-driver/blob/main/test/metabase/driver/exasol_test.clj#L20C10-L26

[andrewdibiasio6]

Something else that is strange, when i check out your branch locally and run the tests, I see a lot more failures.

FAIL in metabase.util.date-2-test/format-human-readable-test (date_2_test.clj:202)

with user locale :en-US java.time.OffsetDateTime #t "2021-04-02T14:42:09.524392-07:00"
expected: (contains? expected actual)
  actual: (not
           (contains?
            #{"April 2, 2021 2:42:09 PM (GMT-07:00)"
              "April 2, 2021, 2:42:09 PM (GMT-07:00)"}
            "April 2, 2021, 2:42:09 PM (GMT-07:00)"))

FAIL in metabase.util.date-2-test/format-human-readable-test (date_2_test.clj:202)

with user locale :en-US java.time.LocalDateTime #t "2021-04-02T14:42:09.524392"
expected: (contains? expected actual)
  actual: (not
           (contains?
            #{"April 2, 2021 2:42:09 PM" "April 2, 2021, 2:42:09 PM"}
            "April 2, 2021, 2:42:09 PM"))

FAIL in metabase.util.date-2-test/format-human-readable-test (date_2_test.clj:203)

with user locale :en-US java.time.OffsetTime #t "14:42:09.524392-07:00"
expected: "2:42:09 PM (GMT-07:00)"
  actual: "2:42:09 PM (GMT-07:00)"

FAIL in metabase.util.date-2-test/format-human-readable-test (date_2_test.clj:203)

with user locale :en-US java.time.LocalTime #t "14:42:09.524392"
expected: "2:42:09 PM"
  actual: "2:42:09 PM"

2944/2977    98% [================================================= ]  ETA: 00:04..instrumented #'metabase.util.malli-test/boo
2944/2977    98% [================================================= ]  ETA: 00:04..instrumented #'metabase.util.malli-test/qux
2944/2977    98% [================================================= ]  ETA: 00:04..instrumented #'metabase.util.malli-test/qux
2944/2977    98% [================================================= ]  ETA: 00:04..instrumented #'metabase.util.malli-test/qux
2944/2977    98% [================================================= ]  ETA: 00:04..instrumented #'metabase.util.malli-test/qux
2944/2977    98% [================================================= ]  ETA: 00:04..instrumented #'metabase.util.malli-test/qux
2944/2977    98% [================================================= ]  ETA: 00:04..instrumented #'metabase.util.malli-test/qux
2944/2977    98% [================================================= ]  ETA: 00:04..instrumented #'metabase.util.malli-test/foo
2946/2977    98% [================================================= ]  ETA: 00:03..instrumented #'metabase.util.malli-test/bar
2946/2977    98% [================================================= ]  ETA: 00:03..instrumented #'metabase.util.malli-test/baz
2971/2977    99% [================================================= ]  ETA: 00:00

Ran 2971 tests in 360.676 seconds
17106 assertions, 15 failures, 0 errors.
{:test 2971,

Am I missing something new?

[Maiquu]

Am I missing something new?

Is your clojure version 1.11.1.1208? Version mismatch is usually the reason (at least for me) for the weird test fails related to java classes.

[andrewdibiasio6]

@Maiquu Metabase team is going to disable that test for us and we should pull their branch.

I may fork your branch and cherry pick that to get these changes in.

[andrewdibiasio6]

Please see: https://github.com/starburstdata/metabase-driver/pull/92

[andrewdibiasio6]

@Maiquu TY for your help! Great work, as always.

I merged your changes in another PR. Metabase disabled that test. I will close this PR :)