-
Notifications
You must be signed in to change notification settings - Fork 638
feat(databricks): add the databricks backend #10223
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.
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
Conversation
Tests won't run until merge due to cloudiness. I'll post the results from a local run, and then fix the CI (if needed) once this is merged. |
d229f75
to
279322d
Compare
b14d84e
to
c833f9b
Compare
ab6762b
to
518e3b1
Compare
This is passing locally:
|
We probably need a policy in place for what happens to backends with no local CI option if financial sponsorship for those CI runs goes away. |
The only reasonable policy I can think of right now is that if funding dries up, the backend goes into backend purgatory, where the CI stops running and support becomes best effort. Thoughts? |
That seems reasonable to me. And I think, if that happens, we add badges to that effect in the readme and on the backend docs page, effectively severing the backend from any semantic versioning guarantees (while still attempting best efforts to maintain those guarantees) |
@gforsyth Where should we put this policy? |
How about a page in the docs in the |
2662993
to
47a7830
Compare
0976530
to
7968c3d
Compare
@gforsyth Finally got around to adding that policy page. |
71fe2ef
to
f9bf147
Compare
Databricks backend passes the backend test suite:
|
Snowflake and BigQuery both passing:
|
I'll try to take a look over this tomorrow! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this looks good to me, modulo the insanity of databricks calling something void
-- just the conflict on the poetry.lock
but then this can go in.
@@ -50,6 +50,7 @@ | |||
"trino": "unknown", | |||
"postgres": "null", | |||
"risingwave": "null", | |||
"databricks": "void", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void
!?
Description of changes
Add support for the databricks backend.
Notes
cases where things differ, and the get overridden in the databricks compiler.
NULL
s intoNaN
s,which defeats a number of array and map tests that expect
None
in the output fromto_pandas
/execute
Naturally, databricks pins pyarrow to <17 (one version behind the latest) and numpy to <2. It's not as bad as it was with the snowflake connector, but we shouldn't merge this until we can figure out a sane workaround to avoid the pin in CI.I worked around this by usingdatabricks-sql-connector-core
which only pinspyarrow
if you depend on it with itspyarrow
extra. Despite the version bounds, the latest version of pyarrow (18 at the time of writing) passes our backend test suite with the databricks backend.Issues closed
Resolves #9248.