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

fix(query): fix double panic if enable_experimental_aggregate_hashtable #14576

Merged
merged 3 commits into from
Feb 3, 2024

Conversation

zhang2014
Copy link
Member

@zhang2014 zhang2014 commented Feb 3, 2024

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

Summary

fix(query): fix double panic if enable_experimental_aggregate_hashtable

  • 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

@github-actions github-actions bot added the pr-bugfix this PR patches a bug in codebase label Feb 3, 2024
@zhang2014 zhang2014 added ci-cloud Build docker image for cloud test and removed pr-bugfix this PR patches a bug in codebase labels Feb 3, 2024
@github-actions github-actions bot added the pr-bugfix this PR patches a bug in codebase label Feb 3, 2024
@zhang2014
Copy link
Member Author

unexpected value in hash_table_payload if enable_experimental_aggregate_hashtable. CC: @sundy-li @Freejww

@BohuTANG
Copy link
Member

BohuTANG commented Feb 3, 2024

It seems we need more unit tests for enable_experimental_aggregate_hashtable=1

@zhang2014 zhang2014 marked this pull request as draft February 3, 2024 04:30
Copy link
Contributor

github-actions bot commented Feb 3, 2024

Docker Image for PR

  • tag: pr-14576-6401662

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

@zhang2014 zhang2014 marked this pull request as ready for review February 3, 2024 04:45
@zhang2014 zhang2014 added ci-cloud Build docker image for cloud test and removed ci-cloud Build docker image for cloud test labels Feb 3, 2024
Copy link
Contributor

github-actions bot commented Feb 3, 2024

Docker Image for PR

  • tag: pr-14576-30ccff7

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

@zhang2014 zhang2014 requested a review from BohuTANG February 3, 2024 05:17
@sundy-li
Copy link
Member

sundy-li commented Feb 3, 2024

How do you catch this error, we don't enable enable_experimental_aggregate_hashtable by default now.

Oh, I found it's enabled as global when testing performance in that tenant.

It seems we need more unit tests for enable_experimental_aggregate_hashtable=1

Yes, but it's not enabled, will cover more tests after we support this with cluster and spill.

@zhang2014 zhang2014 added this pull request to the merge queue Feb 3, 2024
Merged via the queue into databendlabs:main with commit 9323427 Feb 3, 2024
71 checks passed
@zhang2014 zhang2014 deleted the fix/double_panic branch February 3, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-cloud Build docker image for cloud test pr-bugfix this PR patches a bug in codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants