Skip to content
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

Throw error if attempting to delete from table without physical columns #4693

Merged
merged 3 commits into from
Sep 14, 2022

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Sep 13, 2022

Fixes issue #4685

This would previously die in DataChunk::Initialize because it would attempt to create a datachunk without any types.
Since this operation doesn't do anything and in my opinion isn't valid, I have opted to turn this into an error.

@Mytherin
Copy link
Collaborator

Thanks for the PR!

Perhaps the error should instead be on creating a table with only generated columns? I can imagine multiple issues occurring with tables without physical columns (e.g. what about inserts? updates? alter statements?). Such tables don't seem particularly useful to me either.

@Tishj
Copy link
Contributor Author

Tishj commented Sep 13, 2022

Ah good point, address the problem at the source!
I will do that instead 👍

Copy link
Collaborator

@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@Tishj
Copy link
Contributor Author

Tishj commented Sep 14, 2022

I'm not entirely sure why this regression is failing, when I run it locally it also happens for the first init_db (the OLD_DB)
So it seems like it wasn't able to find the old database file ?

@Mytherin
Copy link
Collaborator

Likely a HTTP error, will rerun

@Mytherin Mytherin merged commit 215d3f7 into duckdb:master Sep 14, 2022
@Tishj Tishj deleted the delete_from_non_referencing_gcol_table branch October 5, 2022 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants