-
Notifications
You must be signed in to change notification settings - Fork 332
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
fix: handle insert default value #5307
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5307 +/- ##
==========================================
- Coverage 84.14% 83.89% -0.26%
==========================================
Files 1181 1182 +1
Lines 221141 221529 +388
==========================================
- Hits 186074 185844 -230
- Misses 35067 35685 +618 |
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.
Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (2)
- tests/cases/standalone/common/flow/flow_ins_default.result: Language not supported
- tests/cases/standalone/common/flow/flow_ins_default.sql: Language not supported
Comments suppressed due to low confidence (2)
src/flow/src/adapter/flownode_impl.rs:287
- Ensure that the FetchFromRow enum and its fetch method are covered by tests to verify their behavior.
enum FetchFromRow {
src/flow/src/adapter/table_source.rs:70
- The error message uses incorrect grammar: 'couldn't found'. It should be 'couldn't find'.
TableNotFoundSnafu { name: format!("Table name = {:?}, couldn't found table id", name), }
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 thinking if the default value should be filled in the flownode. For those inpure function (now()
or random()
) as default value, this can lead to wired results. A more proper way in my perspective is filling default value in the frontend before mirroring.
If that's the case, then for i.e. mito engine, wouldn't it mean for dist's datanode, if each datanode have slight derivation in time(Or just different write request arrives in different times), then the default values being filled would be different for different regions in different datanode would have subtle different? Is this consider a bug or feature? see column_default_value function in mito worker? |
4ff32f3
to
886299a
Compare
886299a
to
965aa4f
Compare
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
now()
) in frontend(previously it's on datanode), so for default value likenow()
the result will be consistentFuture works
maybe put
now()
's value into query context, and assign it in frontend, and fill default value using that info in datanode(so that wouldn't need to fill rows in frontend, improving perfmance) , but that would be too large a change to fit into current PR, so future works maybe can be track by #5296 ?PR Checklist
Please convert it to a draft if some of the following conditions are not met.