You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "ArnavBalyan (via GitHub)" <gi...@apache.org> on 2023/10/02 21:20:40 UTC
[PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
ArnavBalyan opened a new pull request, #11725:
URL: https://github.com/apache/pinot/pull/11725
`feature` Support for SQL Formatting in Query Editor
- Add support for Formatting SQL in the query editor UI.
- Added a button under the editor to allow users to format the query.
- Minor adjustments to accommodate the button.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1745780701
> Can you fix the CI pipeline?
Done thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1765166401
> @ArnavBalyan I see a lot of changes in package-lock.json, this project uses npm v5 or lockfile v1 Changing that to npm v6 should not cause issues but we have to test the working.
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1962319117
@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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #11725:
URL: https://github.com/apache/pinot/pull/11725#discussion_r1343213266
##########
pinot-controller/src/main/resources/app/pages/Query.tsx:
##########
@@ -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';
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1765167573
> this is awesome! was there any documentation we can use to follow the usage of sql formatting tool introduced in this PR?
Thanks! I will add some usage instructions to the documentation as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11725:
URL: https://github.com/apache/pinot/pull/11725
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1962317030
> Will allow for easier rollbacks if needed; unless this feature is strictly dependent on this version upgrade.
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1743819086
This looks great! @jayeshchoudhary Can you help take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #11725:
URL: https://github.com/apache/pinot/pull/11725#discussion_r1343203710
##########
pinot-controller/src/main/resources/app/pages/Query.tsx:
##########
@@ -78,7 +79,15 @@ const useStyles = makeStyles((theme) => ({
},
runNowBtn: {
marginLeft: 'auto',
- paddingLeft: '74px',
+ paddingLeft: '10px',
Review 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "jayeshchoudhary (via GitHub)" <gi...@apache.org>.
jayeshchoudhary commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1746934199
@ArnavBalyan I see a lot of changes in package-lock.json, this project uses npm v5 or lockfile v1
Changing that to npm v6 should not cause issues but we have to test the working.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1962314596
> @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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1745394356
Can you fix the CI pipeline?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1743822564
@ArnavBalyan : Pinot supports some unorthodox queries such as [lookUp joins](https://docs.pinot.apache.org/users/user-guide-query/query-syntax/lookup-udf-join), [funnel count](https://github.com/apache/pinot/pull/11092), etc.
Can you test with those as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1744909064
> @ArnavBalyan : Pinot supports some unorthodox queries such as [lookUp joins](https://docs.pinot.apache.org/users/user-guide-query/query-syntax/lookup-udf-join), [funnel count](https://github.com/apache/pinot/pull/11092), etc.
>
> Can you test with those as well?
Hi @ankitsultana it works with lookUp joins and funnel count as well. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "jayeshchoudhary (via GitHub)" <gi...@apache.org>.
jayeshchoudhary commented on code in PR #11725:
URL: https://github.com/apache/pinot/pull/11725#discussion_r1344092460
##########
pinot-controller/src/main/resources/app/pages/Query.tsx:
##########
@@ -78,7 +79,15 @@ const useStyles = makeStyles((theme) => ({
},
runNowBtn: {
marginLeft: 'auto',
- paddingLeft: '74px',
+ paddingLeft: '10px',
Review Comment:
let's at least fix this view in this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1765172512
> Had allowed this change to coming in since upgrading to v2 would give us better performance and stability.
Is it possible to keep version upgrade and this feature PRs separate?
Will allow for easier rollbacks if needed; unless this feature is strictly dependent on this version upgrade.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11725:
URL: https://github.com/apache/pinot/pull/11725#issuecomment-1743846100
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11725?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11725](https://app.codecov.io/gh/apache/pinot/pull/11725?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e3e6e0e) into [master](https://app.codecov.io/gh/apache/pinot/commit/ae1681273c4929587729d71c6dc9526c62ca1fcc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (ae16812) will **increase** coverage by `3.98%`.
> Report is 7 commits behind head on master.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #11725 +/- ##
============================================
+ Coverage 63.11% 67.09% +3.98%
+ Complexity 1117 916 -201
============================================
Files 2342 1747 -595
Lines 125802 91076 -34726
Branches 19336 14633 -4703
============================================
- Hits 79395 61110 -18285
+ Misses 40745 25846 -14899
+ Partials 5662 4120 -1542
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11725/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/11725/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/11725/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/11725/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/11725/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-17](https://app.codecov.io/gh/apache/pinot/pull/11725/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-20](https://app.codecov.io/gh/apache/pinot/pull/11725/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.09% <ø> (+4.14%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11725/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.09% <ø> (+3.98%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11725/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.09% <ø> (+3.99%)` | :arrow_up: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11725/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.09% <ø> (-0.13%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/11725/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
[see 678 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11725/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #11725:
URL: https://github.com/apache/pinot/pull/11725#discussion_r1344039590
##########
pinot-controller/src/main/resources/app/pages/Query.tsx:
##########
@@ -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';
Review Comment:
Hi @ankitsultana the sql-formatter is currently in [maintenance mode](https://github.com/sql-formatter-org/sql-formatter#the-future), no new features are being added. Also it's [incompatible](https://github.com/sql-formatter-org/sql-formatter#module-parse-failed-unexpected-token) 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Feature] Add Support for SQL Formatting in Query Editor [pinot]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #11725:
URL: https://github.com/apache/pinot/pull/11725#discussion_r1344041771
##########
pinot-controller/src/main/resources/app/pages/Query.tsx:
##########
@@ -78,7 +79,15 @@ const useStyles = makeStyles((theme) => ({
},
runNowBtn: {
marginLeft: 'auto',
- paddingLeft: '74px',
+ paddingLeft: '10px',
Review Comment:
Sure, this seems to be something affecting the entire UI, let me work on this in an upcoming change. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org