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

fix: pinot select query logic #9954

Merged
merged 2 commits into from Jun 3, 2020
Merged

fix: pinot select query logic #9954

merged 2 commits into from Jun 3, 2020

Conversation

xiangfu0
Copy link
Contributor

SUMMARY

Update Pinot selection query logic.

  1. Direct return the field if it time grain is None.
  2. No need to remove groupby fields from selection list anymore.

@xiangfu0 xiangfu0 changed the title Update how pinot handles selection logic fix pinot select query logic May 31, 2020
@xiangfu0 xiangfu0 changed the title fix pinot select query logic fix: pinot select query logic May 31, 2020
@codecov-commenter
Copy link

codecov-commenter commented May 31, 2020

Codecov Report

Merging #9954 into master will increase coverage by 0.02%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9954      +/-   ##
==========================================
+ Coverage   71.33%   71.36%   +0.02%     
==========================================
  Files         585      585              
  Lines       30889    30885       -4     
  Branches     3237     3237              
==========================================
+ Hits        22036    22040       +4     
+ Misses       8744     8736       -8     
  Partials      109      109              
Flag Coverage Δ
#cypress 53.98% <ø> (+0.01%) ⬆️
#javascript 59.40% <ø> (ø)
#python 71.55% <57.14%> (+0.02%) ⬆️
Impacted Files Coverage Δ
superset/db_engine_specs/pinot.py 67.56% <57.14%> (+9.03%) ⬆️
.../src/dashboard/components/gridComponents/Chart.jsx 88.76% <0.00%> (-1.13%) ⬇️
superset-frontend/src/SqlLab/actions/sqlLab.js 66.81% <0.00%> (ø)
...rontend/src/visualizations/FilterBox/FilterBox.jsx 76.22% <0.00%> (+3.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fe6f4f...2ad1cca. Read the comment docs.

@mistercrunch
Copy link
Member

mistercrunch commented May 31, 2020

No need to remove groupby fields from selection list anymore.

Is this because it's been addressed in a newer version of Pinot? Do we not case about people on older versions? [Genuine questions] idk how the Pinot community distribution across versions is...

@xiangfu0
Copy link
Contributor Author

xiangfu0 commented Jun 2, 2020

No need to remove groupby fields from selection list anymore.

Is this because it's been addressed in a newer version of Pinot? Do we not case about people on older versions? [Genuine questions] idk how the Pinot community distribution across versions is...

Thanks for bring this up!

In general, it won't change the behavior for old users as old pinot just ignores those fields, no matter the presence and always assume they are there. That's the reason we removed them to make query looks more like Pinot Style.

This code change is for upgrade to use new Pinot Client pinotdb-pypi >=0.3.1, which leverages new Pinot SQL Endpoint(available on Pinot 0.3.0+)

Also if it's possible to know the pinotdb version, then we could potentially keep the old behavior for old users and only enable this for users with newer pinotdb version.

@xiangfu0
Copy link
Contributor Author

xiangfu0 commented Jun 2, 2020

I've also tested this code with both pinotdb 0.2.5 and 0.3.1, both worked.

@mistercrunch mistercrunch merged commit 1d9dbcd into apache:master Jun 3, 2020
@xiangfu0 xiangfu0 deleted the fixing_pinot_selection branch June 7, 2020 18:21
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* Update how pinot handles selection logic

* Change DATETIMECONVERT argument to use single quote for literals
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 0.37.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants