[Feature] Add Support for SQL Formatting in Query Editor #11725
Conversation
|
This looks great! @jayeshchoudhary Can you help take a look? |
|
@ArnavBalyan : Pinot supports some unorthodox queries such as lookUp joins, funnel count, etc. Can you test with those as well? |
| @@ -78,7 +79,15 @@ const useStyles = makeStyles((theme) => ({ | |||
| }, | |||
| runNowBtn: { | |||
| marginLeft: 'auto', | |||
| paddingLeft: '74px', | |||
| paddingLeft: '10px', | |||
There was a problem hiding this comment.
(not introduced in this PR)
Can you check how this looks when the window width is less? At present the buttons and the timeout field start overlapping when we reduce the width. With another button this will become a bit worse.
We can perhaps change the display prop of the divs with some media queries (I have no frontend knowledge so others can chime in)
There was a problem hiding this comment.
Sure, this seems to be something affecting the entire UI, let me work on this in an upcoming change. Thanks!
There was a problem hiding this comment.
let's at least fix this view in this PR
| @@ -47,6 +47,7 @@ import PinotMethodUtils from '../utils/PinotMethodUtils'; | |||
| import '../styles/styles.css'; | |||
| import {Resizable} from "re-resizable"; | |||
| import { useHistory, useLocation } from 'react-router'; | |||
| import sqlFormatter from '@sqltools/formatter'; | |||
There was a problem hiding this comment.
The last update to this package was around a year ago.
I had also found this one earlier which seems to be quite actively maintained: https://www.npmjs.com/package/sql-formatter
Should we explore sql-formatter instead?
There was a problem hiding this comment.
Hi @ankitsultana the sql-formatter is currently in maintenance mode, no new features are being added. Also it's incompatible with the babel configuration we are using. The above sqltools/formatter actively get's maintained and has very close parity to sql-formatter. It has new updates as well. Thanks!
Codecov Report
@@ Coverage Diff @@
## master #11725 +/- ##
============================================
- Coverage 63.11% 63.09% -0.02%
Complexity 1117 1117
============================================
Files 2342 2342
Lines 125802 125890 +88
Branches 19336 19360 +24
============================================
+ Hits 79395 79428 +33
- Misses 40745 40806 +61
+ Partials 5662 5656 -6
Flags with carried forward coverage won't be shown. Click here to find out more. see 61 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Hi @ankitsultana it works with lookUp joins and funnel count and other SQL clauses as well. Thanks! |
|
Can you fix the CI pipeline? |
Done thanks! |
|
@ArnavBalyan I see a lot of changes in package-lock.json, this project uses npm v5 or lockfile v1 |
Got it, on a high level I don't see any issues with the UI, also relying on the tests to make sure nothing breaks. Would you recommend any other way to make sure there are no issues with lockfile v2. Had allowed this change to coming in since upgrading to v2 would give us better performance and stability. Thanks! |
Thanks! I will add some usage instructions to the documentation as well. |
Is it possible to keep version upgrade and this feature PRs separate? |
|
@ArnavBalyan : any updates on this? Can you try to get this closed this month? |
Hey @ankitsultana let me close this in this week, apologies for the delay |
Hey @tibrewalpratik17 this feature is build and tested with lockfile v2. I would suggest we can merge this. In case of rollbacks we can just revert this change, wdyt? Thanks! |
|
@ankitsultana if there are no other concerns, do you think we could merge this, and for the already existing width issue, I can take it up in a future change thanks! |
featuresupport for [feature-request] Add Support for SQL Formatting in Controller Query Editor #11116formatsql2.mov
formatsql1.mov