relferreira / metabase-sparksql-databricks-driver

GNU Affero General Public License v3.0
31 stars 32 forks source link

Support metabase_v0.46 #18

Closed lydiw closed 1 year ago

lydiw commented 1 year ago

I have updated the required files but I don't have time to update the Docker file. I have based on README from this https://github.com/metabase/sudoku-driver repo in order to build the library.

paul-jolimoi commented 1 year ago

I put this here because I struggled a bit following the readme... Hope can help some

If you want to build.

Clone the Metabase repo.

cd /path/to/metabase/
./bin/build

Run this from metabase repo:

DRIVER_PATH=/path/to/driver/repo

clojure -Sdeps "{:aliases {:sparksql-databricks {:extra-deps {com.metabase/sparksql-databricks {:local/root \"$DRIVER_PATH\"}}}}}" -X:build:sparksql-databricks build-drivers.build-driver/build-driver! "{:driver :sparksql-databricks, :project-dir \"$DRIVER_PATH\", :target-dir \"$DRIVER_PATH/target\"}"
bjgbeelen commented 1 year ago

@lydiw thanks for reverting the lower-case-en deletion! This morning tried to build it myself, but I understand that with 0.46.0 the build process for metabase has changed, so haven't been able to verify myself.

I thought the dockerized process that @relferreira introduced (inspired by the athena driver) was pretty sweet, providing an easy way to build and test, without having to clone the metabase repo and requiring all the dependencies to build. That athena driver is no longe rmaintained so no inspiration there :-)

These changes are specific to 0.46.0 I assume, so merging this without also upgrading the Dockerfile doesn't make sense to me. So I would want to keep this open until we've got that sorted out too. I'll see if I can play around and make that work.

bjgbeelen commented 1 year ago

@lydiw I tried compiling the code, but it seems you forgot to include the import to the u reference where lower-case-en can be found

[metabase.util :as u]

I put it above the [metabase.util.date-2 :as u.date]

I have an update locally with the Docker image where the driver gets build and gets included in the latest metabase version (0.46.5). If you can add the import in your branch, I'll create a separate PR for the Dockerfile update. Then we can merge both right after eachother.

Thanks!

danjesus commented 1 year ago

Hey guys I managed to build the images with version v0.46.5, I think we could merge the two Presa into one, does that make sense to you?

alxsbn commented 1 year ago

Can we have some update about it?

bjgbeelen commented 1 year ago

Yes but this one requires changes and I didn't hear back from the original author. I can make the changes hwne i find the time and merge them both

lydiw commented 1 year ago

Yes but this one requires changes and I didn't hear back from the original author. I can make the changes hwne i find the time and merge them both

lydiw commented 1 year ago

Hello,

I believe it's the right approach to merge the two PRs into one. @bjgbeelen what changes are required?

bjgbeelen commented 1 year ago

[metabase.util :as u]

@lydiw this line should be kept, otherwise the driver won't compile. It is used in the reference to the lower-case-en function

lydiw commented 1 year ago

[metabase.util :as u]

@lydiw this line should be kept, otherwise the driver won't compile. It is used in the reference to the lower-case-en function

https://github.com/relferreira/metabase-sparksql-databricks-driver/pull/18/commits/ff033cee3d9e6b3cc84f00a978ae3d481a53812e

bjgbeelen commented 1 year ago

@lydiw would you be able to get that line in? then I'll merge this one and merge the other outstanding PR afterwards

lydiw commented 1 year ago

https://github.com/relferreira/metabase-sparksql-databricks-driver/pull/18/commits/ff033cee3d9e6b3cc84f00a978ae3d481a53812e

It's in this commit. I have added it in June.

bjgbeelen commented 1 year ago

ff033ce

@lydiw oh sorry for that, it must have been my side comparision that caught me thinking it was still unresolved. I now see the line is there below my comments

image