-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(drivers): introduce Apache Pinot #8689
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 8 Skipped Deployments
|
eed65b1
to
13008ef
Compare
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.
@jronsse Thanks for contributing this one! This is really huge! Overall looks great! We need to do only minor polishing I mentioned in comments.
|
||
### Installation | ||
|
||
`npm i @inthememory/pinot-driver` |
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.
We don't need those instructions as it'd be packed as a part of distribution.
} | ||
|
||
public hllInit(sql: string) { | ||
return this.countDistinctApprox(sql); // todo: ensure the correct way to do so in pinot |
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.
This doesn't seem correct so let's remove it for now to avoid weird error messages
} | ||
|
||
public hllMerge(sql: string) { | ||
return this.countDistinctApprox(sql); // todo: ensure the correct way to do so in pinot |
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.
This one as well
|
||
## SSL | ||
|
||
Cube does not require any additional configuration to enable SSL as Elasticsearch connections are made over HTTPS. |
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 guess it should refer to Pinot instead of Elasticsearch?
`npm i @inthememory/pinot-driver` | ||
|
||
### Usage | ||
#### For Docker |
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.
This is as well
|
||
[Learn more](https://github.com/cube-js/cube.js#getting-started) | ||
|
||
### Project Status |
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 guess we don't need it
/** | ||
* @copyright Cube Dev, Inc. | ||
* @license Apache-2.0 | ||
* @fileoverview The `PrestoDriver` and related types declaration. |
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 guess it's PinotDriver instead of PrestoDriver
* feat(driver): introduce Apache Pinot * feat(driver): add Pinot documentation * chore: publish under inthememory scope * chore: clear changelog * chore: rebase master --------- Co-authored-by: Konstantin Burkalev <[email protected]>
Check List
Issue Reference this PR resolves
#8579
Description of Changes Made
The driver uses Pinot's Broker Rest API to submit queries. Multi-stage engine is enabled by default for every query since it offers better SQL support.
Published to npm
npm i @inthememory/pinot-driver