itinycheng / flink-connector-clickhouse

Flink SQL connector for ClickHouse. Support ClickHouseCatalog and read/write primary data, maps, arrays to clickhouse.
Apache License 2.0
373 stars 156 forks source link

[bug fix] fix build sql error in ClickHouseDynamicTableSource #65

Closed Tan-JiaLiang closed 1 year ago

Tan-JiaLiang commented 1 year ago

simlar to FLINK-27268, when projection not match any column, it will return 'ROW<> NOT NULL'.

itinycheng commented 1 year ago

Hi, @Tan-JiaLiang

  1. Code format error found during the code compilation;
  2. Is there any specific scenario for this generated SQL? If this SQL is meaningless, does directly reporting an exception seem more appropriate?
Tan-JiaLiang commented 1 year ago

Hi, @itinycheng

  1. fixed;
  2. I think it was the flink projection bug, when i try the SQL 'SELECT count(*) from ${table}', projection will be return a ROW<> NOT NULL, but i reference to the mysql connector and hive connector, like FLINK-27268, it transform to an empty string. So i think it may need to be fix in connector self.
Tan-JiaLiang commented 1 year ago

By the way, before this bugfix, the "SELECT count(*) FROM ${table}" transform to the operator will be like this: "SELECT FROM ${table}"(source operator which transform to the clickhouse grammar) -> "count operator" -> "collect operator".

after this, transform will be like this: "SELECT '' FROM ${table}" -> "count operator" -> "collect operator".

itinycheng commented 1 year ago

By the way, before this bugfix, the "SELECT count(*) FROM ${table}" transform to the operator will be like this: "SELECT FROM ${table}"(source operator which transform to the clickhouse grammar) -> "count operator" -> "collect operator".

after this, transform will be like this: "SELECT '' FROM ${table}" -> "count operator" -> "collect operator".

@Tan-JiaLiang Yes, I've learned about the issue. In my mind, it more reasonable to add a Rule for the SQL Optimizer(VolcanoPlanner) to slove the parse error of count(*), if we only replace empty with '' in connector it may cause more problems in the future, because we don't know the actual reason why selectFields is empty.

Tan-JiaLiang commented 1 year ago

By the way, before this bugfix, the "SELECT count(*) FROM ${table}" transform to the operator will be like this: "SELECT FROM ${table}"(source operator which transform to the clickhouse grammar) -> "count operator" -> "collect operator". after this, transform will be like this: "SELECT '' FROM ${table}" -> "count operator" -> "collect operator".

@Tan-JiaLiang Yes, I've learned about the issue. In my mind, it more reasonable to add a Rule for the SQL Optimizer(VolcanoPlanner) to slove the parse error of count(*), if we only replace empty with '' in connector it may cause more problems in the future, because we don't know the actual reason why selectFields is empty.

I agree with you. I had reported this bug to the flink official, and it had been fixed in 1.17.0. See FLINK-29558.

itinycheng commented 1 year ago

@Tan-JiaLiang Got it, thank u.