Skip to content
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

Pinot bug fix #1

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Pinot bug fix #1

wants to merge 27 commits into from

Conversation

ege-st
Copy link
Owner

@ege-st ege-st commented Aug 9, 2023

SUMMARY

This fixes two bugs in the Apache Pinot DB Engine spec:

  1. If a column was marked as temporal and no time grain was provided then the query would be constructed with illegal {} around the column name causing Pinot to reject the query as syntactically invalid. The code has been updated to remove the incorrect {}
  2. In some cases, get_timestamp_expr will be called by models.adhoc_column_to_sqla and None is explicitly passed for the pdf parameter. This would cause the Pinot get_timestamp_expr to fault. To fix this, the models.adhoc_column_to_sqla method is updated to get the python_date_format for a column (if that data is available). (Without this fix, queries created for charts simply do not use the date format a user sets for a temporal column).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

  1. Connected to a Pinot data set and marked a long column as epoch_ms and tested creating a Bar Chart V2 to confirm that the query would be correctly constructed (this tested the issue where None is passed for the pdf parameter.
  2. Setting a String column as temporal and using %Y-%m-%d as the PDF value and confirming that when a chart is created the correct Date Time conversion function is used to parse the text date time.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

ege-st pushed a commit that referenced this pull request Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant