-
Notifications
You must be signed in to change notification settings - Fork 609
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
refactor(compilers): move compilers out of the backend dependency path #9590
Conversation
a2a4c9e
to
57103a9
Compare
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.
Looks good to me, pending fixups to the docs build
Ahh, looks like we just need to update this import: from ibis.backends.sql.compiler import ALL_OPERATIONS in the support matrix generation script |
27e1a3d
to
e031937
Compare
@@ -136,6 +135,8 @@ def visit_Cast(self, op, *, arg, to): | |||
if to.is_timestamp(): | |||
return self._to_timestamp(arg, to) | |||
if to.is_decimal(): | |||
from ibis.formats.pyarrow import PyArrowType | |||
|
|||
return self.f.arrow_cast(arg, f"{PyArrowType.from_ibis(to)}".capitalize()) |
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.
This isn't 100% free of dependencies here, because we're using the pyarrow type system for casts, because datafusion doesn't have any other way to represent types than their pyarrow string representation. So, unfortunately, if you're converting from non-datafusion dialects to datafusion then you'll need pyarrow installed.
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.
I think this is a big improvement and this will only trip up users if they are going from non-datafusion dialects to datafusion AND if they have decimal columns.
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.
Yep, it's a somewhat specific use case I think.
Moves the compilers out of the path of backend-specific dependencies, to allow using them with a base installation of Ibis. cc @jonmmease