-
Notifications
You must be signed in to change notification settings - Fork 704
feat: support DELETE with the Java frontend
#962
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
Conversation
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #962 +/- ##
============================================
- Coverage 71.80% 71.72% -0.08%
Complexity 2766 2766
============================================
Files 971 973 +2
Lines 56773 56862 +89
Branches 1787 1790 +3
============================================
+ Hits 40767 40787 +20
- Misses 15116 15184 +68
- Partials 890 891 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
lmatz
left a comment
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.
LGTM
fuyufjh
left a comment
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.
LGTM
| use crate::executor::{BoxedExecutorBuilder, Executor, ExecutorBuilder}; | ||
|
|
||
| /// [`DeleteExecutor`] implements table deletion with values from its child executor. | ||
| // TODO: concurrent `DELETE` may cause problems. A scheduler might be required. |
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.
Indeed. So maybe we have to validate the input stream even for the TableSource (like other CDC sources)? Or introduce a table-level read-write lock? Need some discussions later.
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've created an issue for this, #963.
| ---- | ||
| 114 10 | ||
| 514 20 | ||
| 810 40 |
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.
Stinky numbers as always.
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.
Bugen ❤️ 114514
wyhyhyhyh
left a comment
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.
LSTM
What's changed and what's your intention?
This PR implements
LogicalDeleteandRwBatchDeletein the Java frontend, and add amodify.sltto test the insertions and deletions for table v2.There's a workaround in the frontend named
patchScanfor scanning the hidden column as well for the delete executor.There might be the possibility of bugs for handling
DELETEin the implementations of some other executors. Therefore, the tests in this PR are relatively simple and no other MV is created. We may test and fix those in the next PRs. (#963)Checklist
Refer to a related PR or issue link (optional)
DELETEin TableV2 #16.