-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[Appender] Add AppendDefault
#11905
[Appender] Add AppendDefault
#11905
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.
Thanks for the PR! Great idea. Some comments:
…no DEFAULT expression | ConstantFold where possible | introduce ExpressionExecutor for non foldable expressions
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.
Hey, thanks for the PR! These changes come in handy!
The main functionality I am missing in this PR is writing a default value directly to a vector in a data chunk. Any performance-focused appender implementation most likely uses duckdb_append_data_chunk
instead of appending value-by-value.
From my understanding, in order to write default values to a vector in a data chunk, we need the context of the appender, as we need to know the context of the table. I.e., we cannot add this functionality to the data chunk API. So, maybe it is sensible to extend this PR with a function Appender::AppendDefaultToVector
, which takes a vector and a row index and writes the default value to the expected position... 🤔
That makes sense, I think to be performant it makes more sense to take a list of integers instead of a single row index. |
@taniabogatsch I've made a commit that starts this, taking a SelectionVector and a count for the amount of defaults that need to be appended (taking the indices from the SelectionVector) I've had to make some adjustments to the ExpressionExecutor that make me think this is not it's intended purpose (even though it does already take a |
Not only performance-focused, |
I didn't feel like the This can then be queried for information; |
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.
Hi @Tishj, the changes and improvements look nice! 😄 I've added mostly nits and also some comments.
…ts into the table
@Tishj Will you expose |
…or to include the vector_size
@Giorgi, we plan to expose a way for the appender to write default values when using |
Great! If we don't specify that we want to use default values for some of the columns, will the DuckDB appender automatically use default values for that column? For example, if I have a table with ID auto-increment, will we still need to tell DuckDB that we want to use default for that column? |
Currently, yes. In this first iteration, it is necessary to specify that a value is a default value ( |
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.
Thanks for the changes! I think this PR has too much stuff going on in it at this point - and a few of the things are also not correct and need to be reworked. I propose the following changes to make this PR simpler and get it merged, then we can do follow-up work in subsequent PRs.
Thanks! |
Will append default to vector be implemented in a future PR? The current one isn't very helpful. |
This feature has been requested many times, so I figured it was time to add it.
We add
AppendDefault
to theAppender
class (not BaseAppender).This method can be used in place of
Append(Value val)
The behavior of AppendDefault is the same as calling
INSERT INTO <name> VALUES (DEFAULT)
through SQL