Skip to content

Upgrade minimum supported version to 21.8 in ClickHouse #14126

Merged
ebyhr merged 2 commits intotrinodb:masterfrom
ebyhr:ebi/clickhouse-21.8
Sep 25, 2022
Merged

Upgrade minimum supported version to 21.8 in ClickHouse #14126
ebyhr merged 2 commits intotrinodb:masterfrom
ebyhr:ebi/clickhouse-21.8

Conversation

@ebyhr
Copy link
Copy Markdown
Member

@ebyhr ebyhr commented Sep 14, 2022 •
edited
Loading

Description

Upgrade minimum supported version to 21.8 in ClickHouse
Fixes #14112

Release notes

(x) Release notes are required, with the following suggested text:

# ClickHouse
* Upgrade minimum supported version to 21.8. ({issue}`14112`)

@ebyhr
Remove unhelpful config from ClickHouse type mapping tests
d5fc051
@ebyhr
Upgrade minimum supported version to 21.8 in ClickHouse
adf30d9 
Staying in version 21.8 (EOL is 31 Aug 2022) because
community members may still use the version.
@cla-bot cla-bot bot added the cla-signed label Sep 14, 2022
@github-actions github-actions bot added the docs label Sep 14, 2022
@ebyhr ebyhr requested a review from hashhar September 14, 2022 09:30
hashhar
hashhar reviewed Sep 14, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

I assume the motivation is to cleanup some code since we move from one EOL version to another EOL version?

@tangjiangling
Copy link
Copy Markdown
Member

tangjiangling commented Sep 14, 2022

I assume the motivation is to cleanup some code since we move from one EOL version to another EOL version?

IIUC the main purpose is to support escaping comment values, however unfortunately older versions of ClickHouse do not support table comment. (see #14088 (comment))

@ebyhr
Copy link
Copy Markdown
Member Author

ebyhr commented Sep 14, 2022 •
edited
Loading

I assume the motivation is to cleanup some code

Correct, the diff is +19 −154. We could remove support for 21.8 too, but I feel it's too early.

@ebyhr ebyhr self-assigned this Sep 14, 2022
hashhar
hashhar approved these changes Sep 21, 2022
View reviewed changes
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseTypeMapping.java
.put("metadata.cache-missing", "true")
.buildOrThrow(),
ImmutableList.of());
return createClickHouseQueryRunner(clickhouseServer);
Copy link
Copy Markdown
Member

@hashhar hashhar Sep 21, 2022

Choose a reason for hiding this comment

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

any specific reason to remove metadata cache from type mapping tests?

Copy link
Copy Markdown
Member Author

@ebyhr ebyhr Sep 25, 2022

Choose a reason for hiding this comment

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

Here's elapsed time comparison in my laptop. No improvement by metadata cache.

  • with cache: 1m 36s
  • without cache: 1m 33s

@ebyhr ebyhr merged commit 6eaac39 into trinodb:master Sep 25, 2022
@ebyhr ebyhr deleted the ebi/clickhouse-21.8 branch September 25, 2022 22:53
@github-actions github-actions bot added this to the 398 milestone Sep 25, 2022
@colebow colebow mentioned this pull request Sep 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hashhar hashhar hashhar approved these changes

+1 more reviewer

@tangjiangling tangjiangling tangjiangling approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ebyhr ebyhr

Labels

cla-signed docs

Milestone

398

Development

Successfully merging this pull request may close these issues.

Upgrade minimum supported version in ClickHouse connector

3 participants

@ebyhr @tangjiangling @hashhar