Skip to content

Fix UPSERT binding issue related to the source table_index #6275

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 15, 2023
Merged

Fix UPSERT binding issue related to the source table_index #6275

merged 2 commits into from
Feb 15, 2023

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Feb 14, 2023

This PR fixes #6271

When binding the UPSERT clause, we have to replace the table_index of the BoundColumnReferences, because originally they refer to the table we're inserting into, and not to the "table" we're inserting from.

Previously we assumed to find this table_index in the insert->children[0]->GetTableIndex()[0]
But in the case of LogicalDistinct (and probably others), this LogicalOperator doesn't have a table index.

So now we recursively check child[0] until we find a GetTableIndex() that doesn't return an empty vector.

@Tishj
Don't assume that the first child of the insert has a table index, re…
8918017 
…cursively check the children until we find a table index
@Tishj
Copy link
Contributor Author

Tishj commented Feb 14, 2023 •
edited
Loading

It feels a little hacky, but I'm not sure if there is context enough to determine which child we're interested in.
Could the child operator we're interested in also be a different index in the children vector than 0?

Is the type of the logical operator we're interested in here always the same? (LogicalProjection)

@Mytherin
Copy link
Collaborator

In general INSERT should support any child operator under it. Can't we call GetColumnBindings() on the child to get a list of the childs' column bindings - and then do a rewrite based on those?

@Tishj
Copy link
Contributor Author

Tishj commented Feb 14, 2023 •
edited
Loading

In these expressions, we can refer to the existing table:

  • ON CONFLICT (..) WHERE <expression>
  • DO UPDATE ... WHERE <expression>
  • DO UPDATE SET <expression>

Within these expressions, the BoundColumnReferences that reference table.col or col are bound to the BoundBaseTableRef we created earlier on to verify the INSERT INTO <table>.

The table_index of that Binding is not part of the children, so the ColumnBindingResolver would fail.

If I recall correctly, that's the only reason we do this replacement of table_indexs

@Mytherin Mytherin merged commit cfea2a2 into duckdb:master Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DuckDB CLI v0.7.0: Segmentation fault on "INSERT OR IGNORE"
2 participants
@Tishj @Mytherin