-
Notifications
You must be signed in to change notification settings - Fork 4.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
Adding Apache Pinot Query Runner #5798
Conversation
Just realized the duplicated effort #5724 |
I'm not seeing any movement on #5724 from that contributor. So I'm happy to move forward with this one assuming we can incorporate the feedback from the two previous attempts. The big question was around data type mapping, as I recall. |
085ca51
to
45ab4f2
Compare
45ab4f2
to
153bab5
Compare
Please let me know when you feel this is ready for review 👌 |
Yes, please. Logic wise it's pretty much there. |
@susodapop can you start reviewing this PR, thanks! |
Thanks for the ping! I will review it this week. |
Kindly bump up the thread :p |
Guess I forgot to @susodapop last week :p |
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.
Thank you so much for your patience. This looks great overall! Please see my comments and let me know if you have questions.
for schema_name in self.get_schema_names(): | ||
for table_name in self.get_table_names(): | ||
schema_table_name = "{}.{}".format(schema_name, table_name) | ||
if table_name not in schema: | ||
schema[schema_table_name] = {"name": schema_table_name, "columns": []} | ||
table_schema =self.get_pinot_table_schema(table_name) | ||
|
||
for column in table_schema.get("dimensionFieldSpecs", []) + table_schema.get( | ||
"metricFieldSpecs", []) + table_schema.get("dateTimeFieldSpecs", []): | ||
c = { | ||
"name": column["name"], | ||
"type": PINOT_TYPES_MAPPING[column["dataType"]], | ||
} | ||
schema[schema_table_name]["columns"].append(c) |
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.
Nit: is there a way to pull the entire schema in one roundtrip to Pinot? This approach of looping through schemas -> tables -> columns will work but scales linearly as a schema grows. This isn't a blocker to merge but perhaps a way to incrementally improve in the future.
Looking at the Pinot docs it seems we could pull the entire schema in one roundtrip: https://docs.pinot.apache.org/users/tutorials/schema-evolution#get-the-existing-schema
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.
return list(schema.values()) | ||
|
||
def get_schema_names(self): | ||
return ["default"] |
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.
Does Pinot support schema names other than default
? If so we should make this configurable or fetch them directly from the database.
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.
Right now there is no concept of schema, so just use default
for now.
Later we may consider adding tenant names as part of the schemas.
I appreciate you pinging me so reliably. I've been hard at work on a couple other open source projects for the past month. But I swear I haven't forgotten about you :) |
e3058c9
to
03ea39c
Compare
Thanks for your comments! Seems the CI cannot pass due to some visual change, can you help check what's going on? |
03ea39c
to
4df0c9c
Compare
@susodapop another round :p |
4df0c9c
to
66402ca
Compare
Seems like the CI passed! |
@susodapop ping :p |
kindly Friday ping 🔢 |
Thank you for the pings :) I have been out sick this week. Will review ASAP. |
Oh, hope you get well soon and take your time! |
Merged. Thank you for your patience with this, @xiangfu0 ! |
Many thanks @arikfr and @susodapop !! Great team work! |
* Adding Apache Pinot integration * address comments
* Adding Apache Pinot integration * address comments
* Adding Apache Pinot integration * address comments
What type of PR is this?
Description
Adding Apache Pinot query runner.
How is this tested?
1. Start redash docker-compose
2. Start Pinot Quickstart
This docker run create a Pinot cluster with controller at
http://pinot-quickstart:9000
and broker athttp://pinot-quickstart:8000
3. Create data source:
4. Query Builder
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)