[Feat] Filter & Log Stream Operation #88

IHEII · 2024-01-05T10:13:03Z

Summary

Implement a simple filter
Implement new RPC protocol -- ObTableLSOperation (ObTableLSOperation may be refactored to be more effective and robust in the future)

Solution Description

ShiKaiWi · 2024-01-10T09:37:54Z

src/client/filter.rs

+ * #L%
+ */
+
+#![allow(dead_code)]


Is it necessary to disable dead_code and unused_macros check?

ShiKaiWi · 2024-01-10T09:54:58Z

src/client/filter.rs

+pub struct ObTableValueFilter {
+    pub op: ObCompareOperator,
+    pub column_name: String,
+    pub value: String,


The filter only supports String column filtering (in rust it must be encoded as valid utf-8)?

We use string to pass the message to the server. The OB Server will parse this string to extract the filter ( Filter may be refactored in the future). Although I considered using T to store the value of this filter, the complexity of the existing code necessitates the continued use of a string for this purpose. ( Since we only support ObTableValueFilter on numeric type and string type, using string to store the value of filter is considerable)

we only support ObTableValueFilter on numeric type and string type

If so, I guess some comments about this should be added to this struct.

src/client/filter.rs

ShiKaiWi · 2024-01-17T02:46:42Z

src/client/filter.rs

+pub struct ObTableValueFilter {
+    pub op: ObCompareOperator,
+    pub column_name: String,
+    pub value: String,


we only support ObTableValueFilter on numeric type and string type

If so, I guess some comments about this should be added to this struct.

ShiKaiWi · 2024-01-17T02:48:14Z

src/client/filter.rs

+}
+
+impl ObTableValueFilter {
+    pub fn new<U: ToString, V: ToString>(op: ObCompareOperator, column_name: U, value: V) -> Self {


I guess column_name must be String type, so the U is unnecssary here?
And I guess IntoString may be a better type for value (no clone when the real type is string).

ShiKaiWi · 2024-01-17T02:54:07Z

src/client/filter.rs

+
+const TABLE_COMPARE_FILTER_PREFIX: &str = "TableCompareFilter";
+
+pub trait Filter: Any {


In the rust, there are two ways to achieve the polymorphism:

trait object

enum

And here I find the as_any is used for downcasting, and it's not a good pattern when using trait object. And if the details of the underlying implementation are necessary, enum may be a good choice:

pub enum Filter { Value(ObTableValueFilter), List(ObTableFilterList), }

ShiKaiWi · 2024-01-17T02:56:16Z

src/client/filter.rs

+    fn string(&self) -> String;
+    fn concat_string(&self, filter_string: &mut String);


How about such names:

Suggested change

fn string(&self) -> String;

fn concat_string(&self, filter_string: &mut String);

/// Encode the filter as string.

fn encode(&self) -> String;

/// Encode the filter to the buffer.

fn encode_to&self, buffer: &mut String);

ShiKaiWi · 2024-01-25T06:27:54Z

src/rpc/protocol/query_and_mutate.rs

+        self.flags
+    }
+
+    pub fn set_is_check_and_execute(&mut self, is_check_and_execute: bool) {


Mostly, is_xxx is used for method name, so how about rename it as:

Suggested change

pub fn set_is_check_and_execute(&mut self, is_check_and_execute: bool) {

pub fn set_is_check_and_execute(&mut self, check_and_execute: bool) {

ShiKaiWi · 2024-01-25T06:28:16Z

src/rpc/protocol/query_and_mutate.rs

+        }
+    }
+
+    pub fn set_is_check_not_exists(&mut self, is_check_not_exists: bool) {


Suggested change

pub fn set_is_check_not_exists(&mut self, is_check_not_exists: bool) {

pub fn set_check_not_exists(&mut self, check_not_exists: bool) {

ShiKaiWi · 2024-01-25T06:29:14Z

src/rpc/protocol/query_and_mutate.rs

+}
+
+impl ObTableQueryAndMutate {
+    pub fn internal_new(


If its name is internal_new, shall we make it as private method?

ShiKaiWi · 2024-01-25T07:03:14Z

src/rpc/protocol/payloads.rs

+    pub fn check_and_insert_up<T: FilterEncoder>(
+        &mut self,
+        row_keys: Vec<Value>,
+        columns: Vec<String>,
+        properties: Vec<Value>,
+        filter: T,
+        check_exist: bool,
+    ) {


How about this name:

Suggested change

pub fn check_and_insert_up<T: FilterEncoder>(

&mut self,

row_keys: Vec<Value>,

columns: Vec<String>,

properties: Vec<Value>,

filter: T,

check_exist: bool,

) {

pub fn check_and_upsert(

&mut self,

row_keys: Vec<Value>,

columns: Vec<String>,

properties: Vec<Value>,

filter: impl FilterEncoder,

check_exist: bool,

) {

ShiKaiWi · 2024-01-25T07:04:02Z

src/rpc/protocol/payloads.rs

+    /// check the data with corresponding row_keys
+    /// whether meet the filter and execute the insertUp
+    /// if check_exist is true: check if any data meet the filter
+    /// if check_exist is false: check if all data do not meet the filter


I can't understand the check_exist parameter. The comment about it seems confusing.

After some discussions, I guess we can hide the check_exist parameter, and provide two methods:

check_and_upsert_if_exists check_and_upsert_if_not_exists

And it may lead to less confusion.

ShiKaiWi · 2024-01-25T07:04:57Z

src/client/filter.rs

+#[derive(Debug, Clone)]
+/// Only support [`ObTableValueFilter`] on numeric type and string type
+/// The value will be encoded into string and will be parsed into filter in the server


Suggested change

#[derive(Debug, Clone)]

/// Only support [`ObTableValueFilter`] on numeric type and string type

/// The value will be encoded into string and will be parsed into filter in the server

/// Only support [`ObTableValueFilter`] on numeric type and string type

/// The value will be encoded into string and will be parsed into filter in the server

#[derive(Debug, Clone)]

ShiKaiWi · 2024-01-25T07:26:42Z

src/rpc/protocol/payloads.rs

+    /// check the data with corresponding row_keys
+    /// whether meet the filter and execute the insertUp
+    /// if check_exist is true: check if any data meet the filter
+    /// if check_exist is false: check if all data do not meet the filter


After some discussions, I guess we can hide the check_exist parameter, and provide two methods:

check_and_upsert_if_exists check_and_upsert_if_not_exists

And it may lead to less confusion.

ShiKaiWi · 2024-01-25T07:29:23Z

src/rpc/protocol/payloads.rs

+    // whether a check_and_execute option
+    pub check_and_execute: bool,
+    // check whether any data meet the filter in check_and_execute
+    pub check_exist: bool,


Suggested change

// whether a check_and_execute option

pub check_and_execute: bool,

// check whether any data meet the filter in check_and_execute

pub check_exist: bool,

/// Whether a check_and_execute option

pub check_and_execute: bool,

/// check whether any data meet the filter in check_and_execute

pub check_exists: bool,

BTW, the comment

check whether any data meet the filter in check_and_execute

The check_and_execute seems just a boolean, where is the filter in it?

IHEII added 3 commits January 4, 2024 18:02

[Feat] Filter & Log Stream Operation

b41dd93

[Feat] ObTableLSOperation

fc7a803

[Feat] check and insertUp with filter

d83cd66

ShiKaiWi reviewed Jan 10, 2024

View reviewed changes

IHEII and others added 7 commits January 10, 2024 19:46

Merge branch 'oceanbase:main' into filter

0bdb2e2

[Fix] op type

92d556e

Merge branch 'oceanbase:main' into filter

9931256

Merge branch 'oceanbase:main' into filter

650aa4e

[Fix] review

6d7aa77

[Test] add uint filter test

80e36be

[Chore] format

75057f3

ShiKaiWi reviewed Jan 17, 2024

View reviewed changes

IHEII and others added 3 commits January 17, 2024 16:07

[Fix] review

43979a5

[Chore] modify error message

bd105f7

Merge branch 'oceanbase:main' into filter

e6d3352

ShiKaiWi reviewed Jan 25, 2024

View reviewed changes

[Refactor] check and insertUp API

f4df7f3

IHEII force-pushed the filter branch from 3266c1f to f4df7f3 Compare January 26, 2024 02:36

IHEII and others added 2 commits January 26, 2024 10:36

Merge branch 'oceanbase:main' into filter

eb0fb7d

[Refactor] default atomic flag in batchOperation

82e1f9d

IHEII closed this Mar 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feat] Filter & Log Stream Operation #88

[Feat] Filter & Log Stream Operation #88

IHEII commented Jan 5, 2024

ShiKaiWi Jan 10, 2024

IHEII Jan 15, 2024

ShiKaiWi Jan 10, 2024

IHEII Jan 12, 2024

ShiKaiWi Jan 17, 2024

IHEII Jan 17, 2024

ShiKaiWi Jan 17, 2024

ShiKaiWi Jan 17, 2024 •

edited

Loading

IHEII Jan 17, 2024

ShiKaiWi Jan 17, 2024

IHEII Jan 17, 2024

ShiKaiWi Jan 17, 2024

IHEII Jan 17, 2024

ShiKaiWi Jan 25, 2024

ShiKaiWi Jan 25, 2024

ShiKaiWi Jan 25, 2024

ShiKaiWi Jan 25, 2024

ShiKaiWi Jan 25, 2024

ShiKaiWi Jan 25, 2024

ShiKaiWi Jan 25, 2024

ShiKaiWi Jan 25, 2024

ShiKaiWi Jan 25, 2024


		const TABLE_COMPARE_FILTER_PREFIX: &str = "TableCompareFilter";

		pub trait Filter: Any {

		fn string(&self) -> String;
		fn concat_string(&self, filter_string: &mut String);

-    fn string(&self) -> String;
-    fn concat_string(&self, filter_string: &mut String);
+    /// Encode the filter as string.
+    fn encode(&self) -> String;
+    /// Encode the filter to the buffer.
+    fn encode_to&self, buffer: &mut String);

	pub fn set_is_check_and_execute(&mut self, is_check_and_execute: bool) {
	pub fn set_is_check_and_execute(&mut self, check_and_execute: bool) {

	pub fn set_is_check_not_exists(&mut self, is_check_not_exists: bool) {
	pub fn set_check_not_exists(&mut self, check_not_exists: bool) {

[Feat] Filter & Log Stream Operation #88

[Feat] Filter & Log Stream Operation #88

Conversation

IHEII commented Jan 5, 2024

Summary

Solution Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShiKaiWi Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShiKaiWi Jan 17, 2024 •

edited

Loading