Skip to content

feat: remove filter plan node in pipeline #1126

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.

Sign up for GitHub

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

Jump to bottom
Merged
jiacai2050 merged 6 commits into from
Aug 9, 2023
Merged

feat: remove filter plan node in pipeline #1126

jiacai2050 merged 6 commits into from
Aug 9, 2023

Conversation

@dust1
Copy link
Contributor

@dust1 dust1 commented Aug 3, 2023 •
edited
Loading

Rationale

close #341

Detailed Changes

I just change supports_filter_pushdown in TableProvider to return TableProviderFilterPushDown::Exact to fix this

Test Plan

pass

jiacai2050
jiacai2050 reviewed Aug 3, 2023
View reviewed changes
@dust1
Copy link
Contributor Author

dust1 commented Aug 6, 2023

This issue seems to have been fixed in other pr🤔

@jiacai2050
Copy link
Contributor

jiacai2050 commented Aug 7, 2023

Don't worry, the original issue isn't fixed, I will look into why we can't change to Exact directly.

@jiacai2050
Copy link
Contributor

jiacai2050 commented Aug 7, 2023 •
edited
Loading

I have found the cause why those case failed, in #605 we only allow pushdown primary key filters, so we cannot directly change to Exact here.

@dust1
Copy link
Contributor Author

dust1 commented Aug 7, 2023

ok, I'll check other places to fix the problem

@jiacai2050
Copy link
Contributor

jiacai2050 commented Aug 8, 2023 •
edited
Loading

If we want to return Exact here, we need to ensure following cases are met:

  1. The table is in overwrite mode, and the filter only contains primary key
  2. The table is in append mode

For case 2, you need to update check_and_build_predicate_from_filters to pushdown all filters.
https://github.com/CeresDB/ceresdb/blob/ae17b1f6322fbf3abcc23386d685128b28c1a532/table_engine/src/provider.rs#L186

@jiacai2050
Copy link
Contributor

jiacai2050 commented Aug 9, 2023

@dust1 Your implementation is fine, I will fix CI failed based on your code, please don't push again.

@dust1
Copy link
Contributor Author

dust1 commented Aug 9, 2023

Well, I was gonna fix it later😂

@jiacai2050
Copy link
Contributor

jiacai2050 commented Aug 9, 2023 •
edited
Loading

You can try other issue first, I want to verify this in one of my deployment first.

@jiacai2050 jiacai2050 changed the title edit: remove filter plan node in pipeline feat: remove filter plan node in pipeline Aug 9, 2023
jiacai2050
jiacai2050 approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@jiacai2050 jiacai2050 left a comment

Choose a reason for hiding this comment

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

LGTM

@jiacai2050 jiacai2050 merged commit 50b8f77 into apache:main Aug 9, 2023
@dust1 dust1 deleted the issue341 branch August 10, 2023 02:31
@jiacai2050 jiacai2050 mentioned this pull request Aug 14, 2023
Merged
jiacai2050 added a commit that referenced this pull request Aug 15, 2023
@jiacai2050 @ShiKaiWi
refactor: add support_pushdown in table trait (#1150)
1104866 
## Rationale
Followup PR of #1126, remove hard code condition check .

## Detailed Changes
- Add `support_pushdown` 
- Replace unique_keys with is_unique_column to avoid unnecessary Vec
allocate.
- Remove old memory table pushdown tests, all memory table shouldn't
support pushdown. Pushdown check is ensured in
`integration_tests/cases/common/dml/issue-341.sql`


## Test Plan
Existing tests

---------

Co-authored-by: WEI Xikai <ShiKaiWi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jiacai2050 jiacai2050 jiacai2050 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove filter plan node in pipeline

2 participants

@dust1 @jiacai2050