Skip to content

[CALCITE-6900] Support Char type cast in ClickHouse Dialect#4253

Closed
xuzifu666 wants to merge 1 commit intoapache:mainfrom
xuzifu666:ck_char_support
Closed

[CALCITE-6900] Support Char type cast in ClickHouse Dialect#4253
xuzifu666 wants to merge 1 commit intoapache:mainfrom
xuzifu666:ck_char_support

Conversation

@xuzifu666
Copy link
Member

@xuzifu666 xuzifu666 closed this Mar 19, 2025
@xuzifu666 xuzifu666 reopened this Mar 19, 2025
@xuzifu666 xuzifu666 closed this Mar 19, 2025
@xuzifu666 xuzifu666 reopened this Mar 19, 2025
+ "FROM `foodmart`.`product`";
final String expectedSpark = "SELECT CAST(`product_id` AS CHAR(1))\n"
+ "FROM `foodmart`.`product`";
final String expectedClickHouse = "SELECT CAST(`product_id` AS `FixedString(1)`)\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

assuming this indeed works in Clickhouse, this PR looks fine.
Has this been tested?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, had test for it. sql like:
select cast('a' as FixedString(1))from system.numbers limit 1;
and result return a.

Copy link
Contributor

Choose a reason for hiding this comment

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

for some reason the CI on windows fails

Copy link
Member Author

Choose a reason for hiding this comment

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

Windows ci error may not related to the changed code of this pr,in my repo test is all passed and also build success in local.

Copy link
Member Author

@xuzifu666 xuzifu666 Mar 21, 2025

Choose a reason for hiding this comment

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

1742538835661.png
@mihaibudiu It is the test in my repository and related CI is all passed and based with main branch latest commit.

@mihaibudiu
Copy link
Contributor

I have already approved this.
Hopefully we can make the CI pass as well.

@xuzifu666
Copy link
Member Author

xuzifu666 commented Mar 22, 2025

I have already approved this. Hopefully we can make the CI pass as well.

@mihaibudiu OK, I had open a new PR about it which CI all passed with the same code~ we can handle
on this pr: #4257 If this pr is OK, I would closed this pr.

@mihaibudiu
Copy link
Contributor

I have approved the other one

@mihaibudiu mihaibudiu closed this Mar 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants