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

Web console: Remove first / last suggestions #10794

Merged
merged 2 commits into from
Jan 28, 2021

Conversation

vogievetsky
Copy link
Contributor

@vogievetsky vogievetsky commented Jan 24, 2021

Comment out some optimistic suggestion until such a time when they will be possible at ingestion time.

Related to #10702

image

Copy link
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I left a nit comment on the code style.

Comment on lines 73 to 80
// {
// group: 'first',
// suggestions: ['longFirst', 'doubleFirst', 'floatFirst'],
// },
// {
// group: 'last',
// suggestions: ['longLast', 'doubleLast', 'floatLast'],
// },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't usually allow commented out code in java code. Do you think it's better to be consistent in the web-console as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

@suneet-s
Copy link
Contributor

I don't think this fixes #10702 IMO it should still be an open issue, this fix makes it less likely that someone will trip on this from the UI

@FrankChen021
Copy link
Member

@suneet-s I'm working on #10702. I have finished main coding and unit test cases, and is going to add new IT test cases. But it will take me some time to finish it since I don't get a large chunk of time these days. Once the fix is completed, I will bring back the UI removed by this PR.

@suneet-s
Copy link
Contributor

@suneet-s I'm working on #10702. I have finished main coding and unit test cases, and is going to add new IT test cases. But it will take me some time to finish it since I don't get a large chunk of time these days. Once the fix is completed, I will bring back the UI removed by this PR.

@FrankChen021 that's great news! Looking forward to the PR!

@jihoonson jihoonson merged commit 2a1e47a into apache:master Jan 28, 2021
jihoonson pushed a commit to jihoonson/druid that referenced this pull request Feb 2, 2021
* Remove first / last suggestions

* remove commened out code
jihoonson added a commit that referenced this pull request Feb 3, 2021
* Remove first / last suggestions

* remove commened out code

Co-authored-by: Vadim Ogievetsky <[email protected]>
FrankChen021 added a commit to FrankChen021/druid that referenced this pull request Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants