Skip to content

Add CrossToInnerJoinPass#46408

Merged
kitaisreal merged 8 commits intomasterfrom
vdimir/cross_to_inner_analyzer_0
Feb 24, 2023
Merged

Add CrossToInnerJoinPass#46408
kitaisreal merged 8 commits intomasterfrom
vdimir/cross_to_inner_analyzer_0

Conversation

@vdimir
Copy link
Copy Markdown
Member

@vdimir vdimir commented Feb 14, 2023

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

...

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Feb 14, 2023
@vdimir vdimir mentioned this pull request Feb 14, 2023
Closed
@vdimir vdimir force-pushed the vdimir/cross_to_inner_analyzer_0 branch from e825edb to dd66d68 Compare February 15, 2023 10:49
@kitaisreal kitaisreal self-assigned this Feb 15, 2023
kitaisreal
kitaisreal suggested changes Feb 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@kitaisreal kitaisreal left a comment

Choose a reason for hiding this comment

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

Looks good. Need to add comments, QUERY TREE test and merge.

src/Analyzer/JoinNode.cpp Outdated
throw Exception(ErrorCodes::LOGICAL_ERROR, "Cannot rewrite join {} to inner join, expected cross", toString(kind));

if (children[join_expression_child_index])
throw Exception(ErrorCodes::LOGICAL_ERROR, "Join expression is not empty: '{}'", children[join_expression_child_index]->formatConvertedASTForErrorMessage());
Copy link
Copy Markdown
Contributor

@kitaisreal kitaisreal Feb 17, 2023

Choose a reason for hiding this comment

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

Not sure if this need to be LOGICAL_ERROR, maybe just UNSUPPORTED_METHOD.
Anyway need to add comment to this function in header.

Copy link
Copy Markdown
Member Author

@vdimir vdimir Feb 21, 2023

Choose a reason for hiding this comment

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

I left it as LOGICAL_ERROR because for CROSS join it's expected to have nullptr here, added to comment for function: Expects the current kind to be CROSS (and join expression to be null because of that)

@vdimir vdimir force-pushed the vdimir/cross_to_inner_analyzer_0 branch from dd66d68 to 30c649f Compare February 20, 2023 11:50
@kitaisreal
Copy link
Copy Markdown
Contributor

kitaisreal commented Feb 20, 2023

@vdimir need to fix issue with fast test.

@vdimir
Copy link
Copy Markdown
Member Author

vdimir commented Feb 20, 2023

Hmmm, logs look okay, but timeout 2400s is exceeded 🤔

https://s3.amazonaws.com/clickhouse-test-reports/46408/30c649f7c126a139e0b6783504ebb2dc5e57c159/fast_test/run.log

2023-02-20 15:32:54 354 tests passed. 239 tests skipped. 207.99 s elapsed (MainProcess).
2023-02-20 15:32:55 
2023-02-20 15:32:55 No queries hung.
2023-02-20 15:32:55 All tests have finished.
...
+ clickhouse stop --pid-path /fasttest-workspace/db-fasttest
Command `docker run --cap-add=SYS_PTRACE -e FASTTEST_WORKSPACE=/fasttest-workspace -e FASTTEST_OUTPUT=/test_output -e FASTTEST_SOURCE=/ClickHouse --cap-add=SYS_PTRACE -e PULL_REQUEST_NUMBER=46408 -e COMMIT_SHA=30c649f7c126a139e0b6783504ebb2dc5e57c159 -e COPY_CLICKHOUSE_BINARY_TO_OUTPUT=1 --volume=/home/ubuntu/actions-runner/_work/_temp/fasttest/fasttest-workspace:/fasttest-workspace --volume=/home/ubuntu/actions-runner/_work/_temp/fasttest/fasttest-repo:/ClickHouse --volume=/home/ubuntu/actions-runner/_work/_temp/fasttest/fasttest-output:/test_output --volume=/home/ubuntu/actions-runner/_work/_temp/../ccaches/fasttest:/fasttest-workspace/ccache clickhouse/fasttest:latest`
has failed, timeout 2400s is exceeded

...
2023.02.20 15:33:00.952285 [ 20645 ] {} <Trace> system.transactions_info_log (af447400-a91a-4ade-a9dd-51cebf0f7583): Renaming temporary part tmp_insert_202302_231_231_0 to 202302_231_231_0 with tid (1, 1, 00000000-0000-0000-0000-000000000000).
2023.02.20 15:33:00.952433 [ 20645 ] {} <Trace> SystemLog (system.transactions_info_log): Flushed system log up to offset 43506
2023.02.20 15:33:01.001687 [ 20672 ] {} <Trace> AsynchronousMetrics: MemoryTracking: was 2.22 GiB, peak 8.11 GiB, free memory in arenas 425.71 MiB, will set to 2.18 GiB (RSS), difference: -36.00 MiB
2023.02.20 15:33:03.129839 [ 20404 ] {} <Information> Application: Closed connections. But 22 remain. Tip: To increase wait time add to config: <shutdown_wait_unfinished>60</shutdown_wait_unfinished>
2023.02.20 15:33:03.129895 [ 20404 ] {} <Information> Application: Will shutdown forcefully.
2023.02.20 15:33:03.359881 [ 20402 ] {} <Information> Application: Child process exited normally with code 0.

@vdimir vdimir force-pushed the vdimir/cross_to_inner_analyzer_0 branch from 30c649f to 566221b Compare February 21, 2023 15:38
@vdimir vdimir force-pushed the vdimir/cross_to_inner_analyzer_0 branch from 566221b to 7cf7d31 Compare February 21, 2023 17:28
@kitaisreal
Copy link
Copy Markdown
Contributor

kitaisreal commented Feb 22, 2023

@vdimir all tests failures except build check with clang-tidy are unrelated.

kitaisreal
kitaisreal suggested changes Feb 22, 2023
View reviewed changes
@vdimir vdimir force-pushed the vdimir/cross_to_inner_analyzer_0 branch from ca73187 to b94b446 Compare February 23, 2023 14:06
kitaisreal
kitaisreal approved these changes Feb 24, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@kitaisreal kitaisreal left a comment

Choose a reason for hiding this comment

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

Looks good. Need to address single comment, and then merge.

vdimir
vdimir commented Feb 24, 2023
View reviewed changes
@kitaisreal kitaisreal merged commit b8b399d into master Feb 24, 2023
@kitaisreal kitaisreal deleted the vdimir/cross_to_inner_analyzer_0 branch February 24, 2023 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@kitaisreal kitaisreal kitaisreal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kitaisreal kitaisreal

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vdimir @kitaisreal @robot-ch-test-poll4