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 #5439 bug PrettySpace format #5560

Merged
merged 3 commits into from Jun 9, 2019
Merged

Conversation

stavrolia
Copy link
Contributor

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

For changelog. Remove if this is non-significant change.

  • Bug Fix

Short description (up to few sentences):
Fix infinite loop in pretty space format

@@ -26,12 +25,6 @@ void PrettySpaceBlockOutputStream::write(const Block & block)
Widths name_widths;
calculateWidths(block, widths, max_widths, name_widths, format_settings);

/// Do not align on too long values.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This block affects only appearance, but it breaks the invariant "max_width[i] >= widths[i][j] for each j for each i" that leads to infinite loop at https://github.com/yandex/ClickHouse/blob/master/dbms/src/Formats/PrettyBlockOutputStream.cpp#L202

Subtracting unsigned integers (size_t) is problematic because it can easily lead to overflow (but here it makes this bug visible).

Copy link
Member

Choose a reason for hiding this comment

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

But I want to save this behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

It is really important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain in more details the behaviour?

Copy link
Contributor Author

@stavrolia stavrolia Jun 8, 2019

Choose a reason for hiding this comment

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

Or I can just change result cycling loop from "a < b - c" to "a + c < b" here https://github.com/yandex/ClickHouse/blob/master/dbms/src/Formats/PrettyBlockOutputStream.cpp#L202 . It would save the behavior without infinite loop. @alexey-milovidov is it good in your opinion?

Copy link
Member

Choose a reason for hiding this comment

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

As we have this logic:

https://github.com/yandex/ClickHouse/blob/master/dbms/src/Formats/PrettyBlockOutputStream.cpp#L64

the code you deleted is really obsolete and can be deleted for sure.

@@ -64,14 +57,12 @@ void PrettySpaceBlockOutputStream::write(const Block & block)
}
}
writeCString("\n\n", ostr);

Copy link
Member

Choose a reason for hiding this comment

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

Irrelevant change.

@alexey-milovidov alexey-milovidov merged commit 4c7d71a into master Jun 9, 2019
@stavrolia stavrolia deleted the perftests-file-formats branch June 9, 2019 15:57
@CurtizJ CurtizJ added pr-bugfix Pull request with bugfix, not backported by default v19.8 labels Jun 11, 2019
CurtizJ pushed a commit that referenced this pull request Jun 11, 2019
Fix #5439 bug PrettySpace format

(cherry picked from commit 4c7d71a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants