-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
KYLIN-4491 Support presto pushdown #1209
Conversation
Test PreparePut driver jar to |
Test Pushdown
|
Presto PushDown No matter what data source you use, you can specify the corresponding data source to push down the query
kylin.query.pushdown.runner-class-name=org.apache.kylin.query.pushdown.PushDownRunnerOtherImpl : whether to enable query pushdown kylin.source.jdbc.dialect=presto : specifies JDBC dialect. The default value is default kylin.source.jdbc.adaptor=org.apache.kylin.sdk.datasource.adaptor.PrestoAdaptor : specifies JDBC adaptor |
LGTM |
Codecov Report
@@ Coverage Diff @@
## master #1209 +/- ##
============================================
- Coverage 24.87% 24.83% -0.04%
- Complexity 6264 6266 +2
============================================
Files 1453 1455 +2
Lines 89120 89278 +158
Branches 12438 12454 +16
============================================
+ Hits 22172 22176 +4
- Misses 64771 64927 +156
+ Partials 2177 2175 -2
Continue to review full report at Codecov.
|
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.
Hi @fanfanAlice , could you please share us with your opinion? Thank you.
datasource-sdk/src/main/java/org/apache/kylin/sdk/datasource/adaptor/PrestoAdaptor.java
Show resolved
Hide resolved
import java.util.LinkedList; | ||
import java.util.List; | ||
|
||
public class PushDownRunnerOtherImpl extends AbstractPushdownRunner { |
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.
The name of PushDownRunnerOtherImpl
is not well defined. Since it is the the advanced version of elder PushDownRunnerJdbcImpl
, and reuse some logic in Data Source SDK. Maybe we change is name to something like PushDownRunnerJdbcAdvancedImpl
? What do you think @shaofengshi ?
Codecov Report
@@ Coverage Diff @@
## master #1209 +/- ##
============================================
+ Coverage 24.87% 25.74% +0.86%
- Complexity 6264 6624 +360
============================================
Files 1453 1483 +30
Lines 89120 91053 +1933
Branches 12438 12701 +263
============================================
+ Hits 22172 23441 +1269
- Misses 64771 65317 +546
- Partials 2177 2295 +118
Continue to review full report at Codecov.
|
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 LGTM
Proposed changes
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.
Types of changes
What types of changes does your code introduce to Kylin?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.document
branchFurther comments
If this is a relatively large or complex change, kick off the discussion at user@kylin or dev@kylin by explaining why you chose the solution you did and what alternatives you considered, etc...