Skip to content

Web console: Remove first / last suggestions#10794

Merged
jihoonson merged 2 commits intoapache:masterfrom
implydata:dont-show-first-and-last
Jan 28, 2021
Merged

Web console: Remove first / last suggestions#10794
jihoonson merged 2 commits intoapache:masterfrom
implydata:dont-show-first-and-last

Conversation

@vogievetsky
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 <vadim@ogievetsky.com>
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