-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feature](sql-dialect)support convert hive view and presto view use sql convertor service #46308
Conversation
…ql convertor service
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
…ql convertor service
…ql convertor service
run buildall |
run buildall |
TPC-H: Total hot run time: 32503 ms
|
TPC-DS: Total hot run time: 197395 ms
|
ClickBench: Total hot run time: 31.37 s
|
run buildall |
TPC-H: Total hot run time: 32621 ms
|
TPC-DS: Total hot run time: 197038 ms
|
ClickBench: Total hot run time: 31.36 s
|
run buildall |
TPC-H: Total hot run time: 33157 ms
|
TPC-DS: Total hot run time: 198129 ms
|
ClickBench: Total hot run time: 32.35 s
|
/** | ||
* Convert view definition SQL based on current SQL dialect | ||
*/ | ||
private String convertViewDefinition(String originSql, ConnectContext ctx) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest the extract a common method to do the conversion.
This code is just same as in convertOriginStmt()
in ConnectProcessor.java
* ..... | ||
* } | ||
*/ | ||
if (originalText != null && originalText.contains("/* Presto View: ")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract this logic to a separate method, like parseTrinoViewXXX()
, to make the code more clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
run buildall |
TPC-H: Total hot run time: 32480 ms
|
TPC-DS: Total hot run time: 189782 ms
|
ClickBench: Total hot run time: 32.4 s
|
run buildall |
String trinoViewSql = parseTrinoViewDefinition(originalText); | ||
return trinoViewSql != null ? trinoViewSql : originalText; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not return originalText instead of return null when parse failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. you can return originalText
directly in parseTrinoViewDefinition
's return value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and better add a UT for this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
run buildall |
run buildall |
TPC-H: Total hot run time: 32525 ms
|
TPC-DS: Total hot run time: 195173 ms
|
ClickBench: Total hot run time: 31.92 s
|
External Regression |
run buildall |
TPC-H: Total hot run time: 32568 ms
|
TPC-DS: Total hot run time: 194760 ms
|
ClickBench: Total hot run time: 30.39 s
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR approved by at least one committer and no changes requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
What problem does this PR solve?
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)