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.

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

chore(query): test for enable_experimental_aggregate_hashtable = 1 #14564

Closed
wants to merge 13 commits into from

Conversation

Freejww
Copy link
Contributor

@Freejww Freejww commented Feb 2, 2024

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

Summary

Fix the decimal type mismatch and a bug related to set enable_experimental_aggregate_hashtable=1;.

  • Fixes #[Link the issue here]

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 Reviewable

@Freejww Freejww changed the title fix agghashtable_test chore(query): test for enable_experimental_aggregate_hashtable = 1 Feb 2, 2024
@github-actions github-actions bot added the pr-chore this PR only has small changes that no need to record, like coding styles. label Feb 2, 2024
@Freejww Freejww added ci-benchmark Benchmark: run all test and removed ci-benchmark Benchmark: run all test labels Feb 2, 2024
Copy link
Contributor

github-actions bot commented Feb 2, 2024

Docker Image for PR

  • tag: pr-14564-70e8201

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

@Freejww Freejww marked this pull request as draft February 2, 2024 09:17
@Freejww Freejww marked this pull request as ready for review February 2, 2024 11:18
@@ -149,6 +149,10 @@ impl<Method: HashMethodBounds> AccumulatingTransform for TransformPartialGroupBy
const NAME: &'static str = "TransformPartialGroupBy";

fn transform(&mut self, block: DataBlock) -> Result<Vec<DataBlock>> {
if block.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

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

I think you did not find the key reason for this bug.

Why the tests failed when you enable enable_experimental_aggregate_hashtable yet it succeeded without this config ? There must be some bug inside the new agg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check it

@Freejww Freejww marked this pull request as draft February 3, 2024 01:37
@Freejww Freejww marked this pull request as ready for review February 3, 2024 03:34
@Freejww Freejww marked this pull request as draft February 3, 2024 05:24
@Freejww Freejww marked this pull request as ready for review February 19, 2024 01:07
@sundy-li
Copy link
Member

sundy-li commented Feb 19, 2024

continue in #14544

@sundy-li sundy-li closed this Feb 19, 2024
@Freejww Freejww deleted the fix/agghashtable_test branch March 5, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-chore this PR only has small changes that no need to record, like coding styles.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants