refactor: Optimize When Querying the tables_history Table with the Condition table_id = n #17166

TCeason · 2025-01-03T12:22:35Z

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

Summary

Tests

Unit Test
Logic Test
Benchmark Test
No Test - Explain why

Type of change

Bug Fix (non-breaking change which fixes an issue)
New Feature (non-breaking change which adds functionality)
Breaking Change (fix or feature that could cause existing functionality not to work as expected)
Documentation Update
Refactoring
Performance Improvement
Other (please describe):

This change is

github-actions · 2025-01-03T12:22:53Z

This pull request's title is not fulfill the requirements. @TCeason please update it 🙏.

Valid format:

fix(query): fix group by string bug
  ^         ^---------------------^
  |         |
  |         +-> Summary in present tense.
  |
  +-------> Type: rfc, feat, fix, refactor, ci, docs, chore

Valid types:

rfc: this PR proposes a new RFC
feat: this PR introduces a new feature to the codebase
fix: this PR patches a bug in codebase
refactor: this PR changes the code base without new features or bugfix
ci: this PR changes build/testing/ci steps
docs: this PR changes the documents or websites
chore: this PR only has small changes that no need to record

github-actions · 2025-01-03T12:22:53Z

This pull request's title is not fulfill the requirements. @TCeason please update it 🙏.

Valid format:

fix(query): fix group by string bug
  ^         ^---------------------^
  |         |
  |         +-> Summary in present tense.
  |
  +-------> Type: rfc, feat, fix, refactor, ci, docs, chore

Valid types:

rfc: this PR proposes a new RFC
feat: this PR introduces a new feature to the codebase
fix: this PR patches a bug in codebase
refactor: this PR changes the code base without new features or bugfix
ci: this PR changes build/testing/ci steps
docs: this PR changes the documents or websites
chore: this PR only has small changes that no need to record

github-actions · 2025-01-03T15:37:20Z

Docker Image for PR

tag: pr-17166-385ba4e-1735918577

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

github-actions · 2025-01-03T15:48:51Z

ClickBench Report

internal: https://benchmark.databend.com/clickbench/pr/17166/12599511024/internal.html

drmingdrmer

Reviewed 13 of 14 files at r1, all commit messages.
Reviewable status: 13 of 14 files reviewed, 3 unresolved discussions (waiting on @b41sh and @TCeason)

src/query/storages/system/src/tables_table.rs line 359 at r1 (raw file):

                            .into_iter()
                            .flatten()
                            .filter(|table| !tables_names.contains(table))

Make tables_names a BTreeSet of HashSet. Otherwise for large dataset the O(n^2) time complexity for linear lookup might hang the query.

And it does not need filter() anymore. just call extend() to add new table names.

src/query/storages/system/src/tables_table.rs line 366 at r1 (raw file):

                    Err(err) => {
                        warn!("Failed to get tables: {}, {}", ctl.name(), err);
                    }

Why just warning it but not returning an error? It could be a severe error.

Code quote:

                    Err(err) => {
                        warn!("Failed to get tables: {}, {}", ctl.name(), err);
                    }

src/query/storages/system/src/tables_table.rs line 451 at r1 (raw file):

                            Err(err) => {
                                let msg = format!("Failed to get tables: {}, {}", ctl.name(), err);
                                warn!("{}", msg);

Return the error instead of just warning about it.

Code quote:

                                let msg = format!("Failed to get tables: {}, {}", ctl.name(), err);
                                warn!("{}", msg);

TCeason

Reviewable status: 13 of 14 files reviewed, 3 unresolved discussions (waiting on @b41sh and @drmingdrmer)

src/query/storages/system/src/tables_table.rs line 359 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Make tables_names a BTreeSet of HashSet. Otherwise for large dataset the O(n^2) time complexity for linear lookup might hang the query.

And it does not need filter() anymore. just call extend() to add new table names.

Done.

src/query/storages/system/src/tables_table.rs line 366 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Why just warning it but not returning an error? It could be a severe error.

system tables needs as many output results as possible, and even though the result set may be empty, the business does not want to report errors. If an unexpected result set appears, it can be screened through the log audit function

drmingdrmer

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @b41sh)

src/query/storages/system/src/tables_table.rs line 366 at r1 (raw file):

Previously, TCeason wrote…

system tables needs as many output results as possible, and even though the result set may be empty, the business does not want to report errors. If an unexpected result set appears, it can be screened through the log audit function

Get it

src/query/storages/system/src/tables_table.rs line 451 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Return the error instead of just warning about it.

Same as the above conclusion.

src/query/storages/system/src/tables_table.rs

drmingdrmer

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @b41sh)

github-actions · 2025-01-04T15:38:48Z

Docker Image for PR

tag: pr-17166-7b08800-1736005066

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

github-actions · 2025-01-04T15:52:06Z

ClickBench Report

internal: https://benchmark.databend.com/clickbench/pr/17166/12611477210/internal.html

github-actions · 2025-01-04T23:41:38Z

Docker Image for PR

tag: pr-17166-7b08800-1736034033

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

github-actions · 2025-01-04T23:53:13Z

ClickBench Report

internal: https://benchmark.databend.com/clickbench/pr/17166/12614471466/internal.html

TCeason · 2025-01-05T01:36:09Z

cc @BohuTANG Please merge this pr. All ci pass and was not impact on performance.

…ndition table_id = n

drmingdrmer

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TCeason)

atifiu · 2025-01-08T14:44:48Z

I can confirm that issue is fixed in version 1.2.685. Total time to fetch is approx. 0.06 secs as compared to version 1.2.674 where it took 160+ secs

TCeason marked this pull request as draft January 3, 2025 12:22

TCeason force-pushed the history branch from 0154edf to 4255f65 Compare January 3, 2025 12:23

TCeason changed the title ~~optimize: Optimize When Querying the tables_history Table with the Condition table_id = n~~ refactor: Optimize When Querying the tables_history Table with the Condition table_id = n Jan 3, 2025

github-actions bot added the pr-refactor this PR changes the code base without new features or bugfix label Jan 3, 2025

TCeason requested review from drmingdrmer and b41sh January 3, 2025 14:12

TCeason marked this pull request as ready for review January 3, 2025 14:12

TCeason added the ci-benchmark-local Benchmark: run only local test label Jan 3, 2025

drmingdrmer requested changes Jan 4, 2025

View reviewed changes

TCeason commented Jan 4, 2025

View reviewed changes

TCeason force-pushed the history branch from 7529f82 to d54ae81 Compare January 4, 2025 11:56

drmingdrmer approved these changes Jan 4, 2025

View reviewed changes

TCeason commented Jan 4, 2025

View reviewed changes

src/query/storages/system/src/tables_table.rs Outdated Show resolved Hide resolved

drmingdrmer approved these changes Jan 4, 2025

View reviewed changes

b41sh approved these changes Jan 4, 2025

View reviewed changes

TCeason added ci-benchmark-local Benchmark: run only local test and removed ci-benchmark-local Benchmark: run only local test labels Jan 4, 2025

TCeason added 2 commits January 5, 2025 10:15

optimize: Optimize When Querying the tables_history Table with the Co…

f008f4b

…ndition table_id = n

use btreeset replace vec

c07f0af

ignore check contain and add some comment

488ea62

drmingdrmer force-pushed the history branch from 00832bd to 488ea62 Compare January 5, 2025 02:15

drmingdrmer approved these changes Jan 5, 2025

View reviewed changes

drmingdrmer added this pull request to the merge queue Jan 5, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 5, 2025

drmingdrmer added this pull request to the merge queue Jan 5, 2025

BohuTANG removed this pull request from the merge queue due to a manual request Jan 5, 2025

BohuTANG merged commit 8f6713d into databendlabs:main Jan 5, 2025
73 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Optimize When Querying the tables_history Table with the Condition table_id = n #17166

refactor: Optimize When Querying the tables_history Table with the Condition table_id = n #17166

TCeason commented Jan 3, 2025 •

edited

Loading

github-actions bot commented Jan 3, 2025

github-actions bot commented Jan 3, 2025

github-actions bot commented Jan 3, 2025

github-actions bot commented Jan 3, 2025

drmingdrmer left a comment

TCeason left a comment

drmingdrmer left a comment

drmingdrmer left a comment

github-actions bot commented Jan 4, 2025

github-actions bot commented Jan 4, 2025

github-actions bot commented Jan 4, 2025

github-actions bot commented Jan 4, 2025

TCeason commented Jan 5, 2025

drmingdrmer left a comment

atifiu commented Jan 8, 2025

refactor: Optimize When Querying the tables_history Table with the Condition table_id = n #17166

refactor: Optimize When Querying the tables_history Table with the Condition table_id = n #17166

Conversation

TCeason commented Jan 3, 2025 • edited Loading

Summary

Tests

Type of change

github-actions bot commented Jan 3, 2025

github-actions bot commented Jan 3, 2025

github-actions bot commented Jan 3, 2025

Docker Image for PR

github-actions bot commented Jan 3, 2025

ClickBench Report

drmingdrmer left a comment

Choose a reason for hiding this comment

TCeason left a comment

Choose a reason for hiding this comment

drmingdrmer left a comment

Choose a reason for hiding this comment

drmingdrmer left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 4, 2025

Docker Image for PR

github-actions bot commented Jan 4, 2025

ClickBench Report

github-actions bot commented Jan 4, 2025

Docker Image for PR

github-actions bot commented Jan 4, 2025

ClickBench Report

TCeason commented Jan 5, 2025

drmingdrmer left a comment

Choose a reason for hiding this comment

atifiu commented Jan 8, 2025

TCeason commented Jan 3, 2025 •

edited

Loading