Skip to content

feat: Support DeleteRecord message#18

Merged
kodiakhq[bot] merged 6 commits intomainfrom
support-delete
Oct 12, 2023
Merged

feat: Support DeleteRecord message#18
kodiakhq[bot] merged 6 commits intomainfrom
support-delete

Conversation

@bbernays
Copy link
Copy Markdown
Contributor

Implements structure laid out in RFC

@bbernays bbernays requested a review from yevgenypats as a code owner October 10, 2023 19:10

message PredicatesGroup {
enum GroupingType {
AND = 0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can start with only AND as we're treating this as remove row by PK/full row contents

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So what is the path forward to support OR grouping?

OR = 1;
}
GroupingType grouping_type = 1;
repeated Predicate predicates = 2;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel like this is an overkill.

Instead, we need to supply column list + record to compare against.
This way, if the column list is empty, we just need to compare whole record, but if it isn'n then we just compare the values to the ones from the supplied single record.

(now you supply record per column, I suggest supplying single record (=row) & just indicate what columns are required for comparison)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This structure allows for AND and OR grouping of predicates.

Copy link
Copy Markdown
Contributor

@candiduslynx candiduslynx Oct 12, 2023

Choose a reason for hiding this comment

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

IMO we should just use expression syntax, like:

Expression : UnaryExpression | BinaryExpression
UnaryExpression: Not Expression | Value | ColumnName
BinaryExpression: UnaryExpression BinaryOp UnaryExpression
BinaryOp: Comparison | AND | OR
Comparison: > | >= | < | <= | ==

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Example: #19

Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

@bbernays bbernays added the automerge Add to automerge PRs once requirements are met label Oct 12, 2023
@bbernays bbernays changed the title feat: Support DeleteRecord feat: Support DeleteRecord message Oct 12, 2023
@kodiakhq kodiakhq bot merged commit d0a60d6 into main Oct 12, 2023
@kodiakhq kodiakhq bot deleted the support-delete branch October 12, 2023 16:20
kodiakhq bot pushed a commit to cloudquery/plugin-sdk that referenced this pull request Oct 13, 2023
#### Summary

Blocked by cloudquery/plugin-pb#18

(linting and tests fail because of protobuf isn't updated)
kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this pull request Oct 16, 2023
hydratim pushed a commit to hydratim/cloudquery that referenced this pull request Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Add to automerge PRs once requirements are met

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants