Skip to content

better scalar query#7392

Merged
4ertus2 merged 1 commit intoClickHouse:masterfrom
amosbird:scalar
Oct 23, 2019
Merged

better scalar query#7392
4ertus2 merged 1 commit intoClickHouse:masterfrom
amosbird:scalar

Conversation

@amosbird
Copy link
Collaborator

@amosbird amosbird commented Oct 19, 2019 •
edited by 4ertus2
Loading

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Category (leave one):

  • Performance Improvement

Short description (up to few sentences):
Using column instead of AST to store scalar query results.

@yuzhichang @yingfeng

@yingfeng
Copy link

yingfeng commented Oct 20, 2019

Awesome job!

@amosbird
Copy link
Collaborator Author

amosbird commented Oct 20, 2019

To-do , use generic ASTLiteral instead of ASTFunction

@amosbird amosbird force-pushed the scalar branch 7 times, most recently from e4e105e to 5f7b2c8 Compare October 21, 2019 15:46
@amosbird
Copy link
Collaborator Author

amosbird commented Oct 22, 2019

perf test shows promising results

@yuzhichang
Copy link
Contributor

yuzhichang commented Oct 22, 2019

Great job! Tested with product data, found no problem.

@amosbird amosbird mentioned this pull request Oct 22, 2019
Merged
4ertus2
4ertus2 reviewed Oct 22, 2019
View reviewed changes
4ertus2
4ertus2 reviewed Oct 22, 2019
View reviewed changes
@4ertus2
Copy link
Contributor

4ertus2 commented Oct 22, 2019 •
edited
Loading

I've updated PR description. We use template strings in scripts. PR should have "Category" and "Short description" lines present and unchanged.

@amosbird amosbird force-pushed the scalar branch 2 times, most recently from cae68dc to 29f9223 Compare October 23, 2019 09:44
@4ertus2 4ertus2 merged commit 18a72fa into ClickHouse:master Oct 23, 2019
@amosbird
Copy link
Collaborator Author

amosbird commented Oct 24, 2019

enable_scalar_subquery_optimization is set to true by default in order to get the perf test result. So there is a backward incompatibility and one might update the settings profile and set it to false so that a cluster installation can be upgraded without downtime.

@akuzm akuzm added the pr-improvement Pull request with some product improvements label Oct 29, 2019
azat added a commit to azat/ClickHouse that referenced this pull request Nov 3, 2019
@azat
Check column for __getScalar() (avoids assertion)
842f4e2 
Can be triggered using the following query:
  CREATE TABLE foo (key String, macro String MATERIALIZED __getScalar(key)) Engine=Null();

Trace:
    3. 0x00007ffff6d5d526 __assert_fail (libc.so.6)
    4. 0x00007ffff41fd931 boost::intrusive_ptr<DB::IColumn const>::operator*() const (libclickhouse_functionsd.so)
    5. 0x00007ffff41fcd64 COW<DB::IColumn>::IntrusivePtr<DB::IColumn const>::operator*() const & (libclickhouse_functionsd.so)
    6. 0x00007ffff4dc5944 DB::FunctionGetScalar::getReturnTypeImpl() const (libclickhouse_functionsd.so)

(Even though it is internal I guess it is better to fix it)

Refs: ClickHouse#7392
Cc: @amosbird
@azat azat mentioned this pull request Nov 3, 2019
Merged
@SaltTan
Copy link
Contributor

SaltTan commented Nov 14, 2019

If I set enable_scalar_subquery_optimization to anything I will get Exception: Unknown setting enable_scalar_subquery_optimization from older versions in distributed queries.

@den-crane
Copy link
Contributor

den-crane commented Nov 14, 2019

enable_scalar_subquery_optimization is set to true

@amosbird you made upgrade of a cluster is impossible.

@amosbird
Copy link
Collaborator Author

amosbird commented Nov 15, 2019

@SaltTan but enable_scalar_subquery_optimization only affects newer version of clickhouse. Older versions should behave as if it is set to false

@den-crane Hmm, did you try upgrading with that setting = false using config file?

@den-crane
Copy link
Contributor

den-crane commented Nov 15, 2019 •
edited
Loading

@amosbird
= false Yes, the upgraded server starts to respond with errors Exception: Unknown setting enable_scalar_subquery_optimization because not upgraded shards know nothing about this setting.

You have to change default value to false as always for new settings.

@amosbird
Copy link
Collaborator Author

amosbird commented Nov 15, 2019

Ok, I'm not familiar with cluster upgrading, especially seemingless ones. If defaulting the setting to false is a proper fix, I'm fine with that. As I said, "enable_scalar_subquery_optimization is set to true by default in order to get the perf test result."

@SaltTan SaltTan mentioned this pull request Nov 19, 2019
Closed
@CurtizJ CurtizJ mentioned this pull request Dec 22, 2019
Closed
@amosbird amosbird mentioned this pull request Sep 24, 2020
Closed
@amosbird amosbird mentioned this pull request Apr 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@4ertus2 4ertus2 4ertus2 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-improvement Pull request with some product improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@amosbird @yingfeng @yuzhichang @4ertus2 @SaltTan @den-crane @akuzm