[GLUTEN-9177][CH]Fix diff on parse host of url and refactor SparkParseURL#9179
[GLUTEN-9177][CH]Fix diff on parse host of url and refactor SparkParseURL#9179taiyang-li merged 1 commit intoapache:mainfrom
SparkParseURL#9179Conversation
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
KevinyhZou
force-pushed
the
fix_diff_parse_url
branch
from
e025d06 to
bff9ac2
Compare
|
Run Gluten Clickhouse CI on x86 |
| } | ||
|
|
||
| test("GLUTEN-9177: Fix diff of parse_url") { | ||
| val select_sql = "select id, parse_url('http://user:pass@locahost', 'HOST'), " + |
There was a problem hiding this comment.
const expression is still evaluated in vanilla spark although gluten is enabled.
| } | ||
| else | ||
| { | ||
| host = std::string_view{}; |
There was a problem hiding this comment.
let host as it be. It looks redundant here
| @@ -380,6 +380,21 @@ struct SparkExtractURLHost | |||
| if (userinfo_delim_pos && userinfo_delim_pos < end) | |||
There was a problem hiding this comment.
instead of parsing url manually, why not use Poco::URI? It should be much easier than current implementation.
There was a problem hiding this comment.
The implementation of SparkParseURL.cpp is seems try to use the url parsing function of clickhouse, and I think it is may be better than Poco::URI, if Poco::URI is a better way, I think ch would use it by default .
There was a problem hiding this comment.
But the existing code looks a bit messy and will be difficult to maintain in the future. Would you mind investigating the feasibility of Poco::URI?
There was a problem hiding this comment.
Besides, make sure that all related spark uts are enabled.
KevinyhZou
force-pushed
the
fix_diff_parse_url
branch
from
bff9ac2 to
88a0b5a
Compare
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
KevinyhZou
force-pushed
the
fix_diff_parse_url
branch
from
4b566d4 to
5e1e429
Compare
|
Run Gluten Clickhouse CI on x86 |
KevinyhZou
changed the title
SparkParseURL
2 participants
What changes were proposed in this pull request?
(Please fill in changes proposed in this fix)
(Fixes: #9177)
How was this patch tested?
test by ut