Skip to content
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

chore: remove the static filter for merge into #14011

Closed
wants to merge 20 commits into from
Closed

chore: remove the static filter for merge into #14011

wants to merge 20 commits into from

Conversation

JackTan25
Copy link
Contributor

@JackTan25 JackTan25 commented Dec 14, 2023 •
edited
Loading

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

Summary about this PR

    1. I just add more comments and tests for cluster table, it's to prevent people from modifying the
      logic in the future.
    1. remove static filter

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

This change is Reviewable

@JackTan25 JackTan25 requested a review from dantengsky December 14, 2023 05:47
@JackTan25 JackTan25 changed the title test: add more cluster test ci: add more cluster test Dec 14, 2023
@github-actions github-actions bot added the pr-build this PR changes build/testing/ci steps label Dec 14, 2023
@JackTan25 JackTan25 marked this pull request as ready for review December 14, 2023 05:52
@JackTan25 JackTan25 marked this pull request as draft December 14, 2023 06:30
@JackTan25

This comment was marked as outdated.

Sign in to view
@JackTan25 JackTan25 requested a review from SkyFan2002 December 14, 2023 07:08
@JackTan25
Copy link
Contributor Author

@SkyFan2002 will remove static filter in the future. But let's fix this firstly.

@JackTan25 JackTan25 marked this pull request as ready for review December 14, 2023 11:23
dantengsky
dantengsky previously approved these changes Dec 14, 2023
View reviewed changes
@dantengsky dantengsky dismissed their stale review December 14, 2023 14:18

sqlogic test failed

@JackTan25 JackTan25 added the ci-cloud Build docker image for cloud test label Dec 14, 2023
@JackTan25
Copy link
Contributor Author

let me test cloud and check wizard cc @dantengsky @SkyFan2002

@JackTan25 JackTan25 added ci-cloud Build docker image for cloud test and removed ci-cloud Build docker image for cloud test labels Dec 14, 2023
@github-actions GitHub Actions
Copy link
Contributor

Docker Image for PR

  • tag: pr-14011-36307fa

note: this image tag is only available for internal use,
please check the internal doc for more details.

@BohuTANG BohuTANG requested a review from dantengsky December 15, 2023 06:48
@JackTan25 JackTan25 mentioned this pull request Dec 15, 2023
Closed
@BohuTANG
Copy link
Member

Conflicting files
src/query/service/src/interpreters/interpreter_merge_into_static_filter.rs

@JackTan25 JackTan25 added ci-cloud Build docker image for cloud test and removed ci-cloud Build docker image for cloud test labels Dec 18, 2023
@github-actions GitHub Actions
Copy link
Contributor

Docker Image for PR

  • tag: pr-14011-b8b13be

note: this image tag is only available for internal use,
please check the internal doc for more details.

@JackTan25
Copy link
Contributor Author

The merge into wizard test result can see in #13950 (it removes static filter, but the pr is waiting this pr).

@BohuTANG BohuTANG changed the title ci: add more cluster test chore: remove the static filter for merge into Dec 18, 2023
@github-actions github-actions bot added the pr-chore this PR only has small changes that no need to record, like coding styles. label Dec 18, 2023
@BohuTANG
Copy link
Member

BohuTANG commented Dec 18, 2023 •
edited
Loading

This PR is important for the MERGEINTO performance, I modify the PR title to make it more sense, and we won't make this PR into the next stable version(we are prepare for it), so the review and merge will more later.

@github-actions GitHub Actions
Copy link
Contributor

Docker Image for PR

  • tag: pr-14011-72c2f0f

note: this image tag is only available for internal use,
please check the internal doc for more details.

@BohuTANG
Copy link
Member

@dantengsky @JackTan25
Can we don't remove the static filter(due to the performance) and design the new runtime filter? When new runtime filter is ready then remove the static filter.

@JackTan25
Copy link
Contributor Author

@dantengsky @JackTan25
Can we don't remove the static filter(due to the performance) and design the new runtime filter? When new runtime filter is ready then remove the static filter.

we will dive into a trouble (the static filter implementation is hack, it can't pass the tests for this pr, maybe I can overcome these, but I'm not sure there could be many potential cases, however it can process some normal cases that we need for now).It's up to you, cc @dantengsky

dantengsky
dantengsky previously approved these changes Dec 20, 2023
View reviewed changes
@dantengsky dantengsky added this pull request to the merge queue Dec 20, 2023
@dantengsky dantengsky removed this pull request from the merge queue due to a manual request Dec 20, 2023
@dantengsky dantengsky dismissed their stale review December 20, 2023 05:23

cease merging, trying to move this PR to new dev branch

@dantengsky dantengsky mentioned this pull request Dec 20, 2023
Merged
11 tasks
@dantengsky
Copy link
Member

re-opened at #14092

@dantengsky dantengsky closed this Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dantengsky dantengsky dantengsky left review comments

@SkyFan2002 SkyFan2002 Awaiting requested review from SkyFan2002

Assignees
No one assigned
Labels
ci-cloud Build docker image for cloud test dont-merge-for-next-stable-release pr-build this PR changes build/testing/ci steps pr-chore this PR only has small changes that no need to record, like coding styles.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JackTan25 @BohuTANG @dantengsky