-
Notifications
You must be signed in to change notification settings - Fork 610
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(sql): support inserts with default constraints #9844
feat(sql): support inserts with default constraints #9844
Conversation
fc06dbe
to
5c6d1eb
Compare
I have made more progress on the tests. I got the Oracle backend passing, but it required a hacky solution to ensure the identifier was being quoted. import ibis.backends.sql.compilers as sc
quoted = getattr(sc, con.dialect.__name__.lower()).compiler.quoted It works, but I wonder if there is a better way. I will try to look into the remaining backends that are still failing, which are: Clickhouse, Trino, Druid, Exasol. Edit: |
ade767e
to
dc4a244
Compare
ibis/backends/sql/__init__.py
Outdated
columns=[ | ||
sg.to_identifier(col, quoted=quoted) | ||
for col in columns | ||
if col in source_cols |
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.
Let's make a variable for source.schema
and then use col in variable
to do this lookup.
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 believe the change I have made will reduce the time complexity to avoid needing to check again against the source_cols list. I have rewritten things a bit here, using a variable named columns
to refer to the column list to be used for the insert SQL expression.
b302ab5
to
258271f
Compare
I swapped things around here to use
So if my source schema is: ibis.Schema {
b string
a int64
} But my target schema is: ibis.Schema {
a int64
b string
} This is okay, because the underlying SQL expression come out to: INSERT INTO target (b, a)
SELECT *
FROM source; and in the event that maybe the target schema has a default constraint on column "a", and our source schema looks like ibis.Schema {
b string
} the query will be written like so: INSERT INTO target (b)
SELECT *
FROM source; |
I have marked this one ready for review, but I have a couple of questions.
|
8c0a4b8
to
cabd583
Compare
I'll add an xfail marker to the Druid backend, since it doesn't support |
Oh, looks like you did that already. Let me see what's failing then. |
@IndexSeek I improved the implementation bit: we can use our This avoids constructing two throwaway |
This is a very nice improvement. Thank you for the assistance here and the explanation, @cpcloud! |
Running the cloud backend test suite, then if that's all green this is good to merge! |
try: | ||
db = getattr(con, "current_database", None) | ||
except NotImplementedError: | ||
db = None |
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'm going to put up a separate PR to fix the reason why this is so gross.
Description of changes
This change intends to support scenarios where a user needs to insert into a table where a table contains DEFAULT values on columns, meaning the incoming object may have fewer columns than the target table schema.
It's common to insert into tables with DEFAULT constraints; in many cases, these are sequences, and it is a bit tricky to provide the sequence "nextval" approach today.
This topic was initially brought up in Zulip.
Here is an example of this in practice:
Backend error
If the user excludes a required column (e.g., NOT NULL without a DEFAULT) from the container used for insert, the backend will error out with its respective error.
Successful insert with excluded columns
I'm still working through the tests for this, but wanted to put this out here in case anyone wanted to take a look before I can return to it.