-
Notifications
You must be signed in to change notification settings - Fork 147
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
[FEAT] Initializes daft-sql and defines the daft.sql(..) function. #2559
Conversation
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.
Nice! I saw that we started to overlap on the Rust side — you were able to get a lot done so quickly. If you want to focus on the Rust side, I will rebase from #2558 when I have bandwidth. Otherwise, you are welcome to take the Python frontend things you wish and change up the names like SQLCatalog->SQLContext. |
import pytest | ||
|
||
import daft | ||
from daft.sql.sql import SQLCatalog |
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.
nit: is there any way we can have this just from daft.sql
instead of `from daft.sql.sql
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 totally agree, but struggled without moving existing things. I had a circular import error by adding things in sql.py
to the sql/__init__.py
because of the existing files.
Is moving sql_scan and sql_connection to avoid the circular dependency a viable option?
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.
a few small nits, but overall looks good! great work @RCHowell!
This pull request adds the daft-sql module to the Rust side, and defines a
daft.sql(..)
method along with a placeholder test.Then intention of this PR is to establish the structure and plumbing before introducing any functionality.