Allow decimal values in settings using seconds by Algunenano · Pull Request #37187 · ClickHouse/ClickHouse

Algunenano · 2022-05-13T12:58:24Z

Changelog category (leave one):

Backward Incompatible Change

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Changes how settings using seconds as type are parsed to support floating point values (for example: max_execution_time=0.5). Infinity or NaN values will throw an exception.

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

Maintains milliseconds as it was.
Closes #36940

Algunenano · 2022-05-13T15:22:04Z

Apparently there are settings (drain_timeout) with seconds where negative values are ok (and have a meaning) so I'll remove that limitation.

Algunenano · 2022-05-13T21:09:24Z

clang-tidy is complaining (without reason) but it can be workaround.
arm64 tests are failing at 01287_max_execution_speed which look unrelated since it's not using any seconds-unit settings.

Algunenano · 2022-05-13T23:17:59Z

arm64 tests are failing at 01287_max_execution_speed which look unrelated since it's not using any seconds-unit settings.

Ok, nevermind. That test is setting

SET min_execution_speed_bytes = 800000000000, timeout_before_checking_execution_speed = 0.1;

The timeout_before_checking_execution_speed=0.1, as far as I can see, has never worked because it got truncated to 0. With the changes it respects the 0.1 and, as expected, will not check the execution time until it reaches 0.1 seconds. I guess that's why the test is failing.

I could find that the following tests are setting timeout_before_checking_execution_speed to a decimal value:

01287_max_execution_speed.sql
00156_max_execution_speed_sample_merge.sql
01290_max_execution_speed_distributed.sql

I'm going to lower their values to 0 since that's what they were effectively using before.

Before the change to use floats, this settings were getting 0 as the parsed value, so keep the old value by setting to zero explicitly

Algunenano · 2022-05-14T13:02:16Z

Tests:

Stateless tests flaky check (address, actions): 01287_max_execution_speed fails because it's too long (under asan)
Stress tests: They are complaining about backward compatibility checks but I'm not sure what I'm supposed to be seeing / looking for there.

yakov-olkhovskiy · 2022-05-23T19:23:50Z

src/Core/SettingsFields.cpp

This change can be source of incompatibility for some weird cases, where we rely on integer. But overall shouldn't be an issue, I believe.

If you have any reservation about a specific case let me know and I'll add some extra tests to make sure everything works as expected.

frankly can't find anything at least in code... otherwise it will be a proper conversion I think.

yakov-olkhovskiy · 2022-05-23T19:29:08Z

src/Core/SettingsFields.cpp

I think it's better to comment it in terms of lost precision for seconds due to requirements for backward compatibility - we will lose context of this PR in source code - will be not clear unchanged from what.

True. I'll improve the comment.

yakov-olkhovskiy · 2022-05-24T14:32:07Z

@Algunenano 01287_max_execution_speed is failing due to different output
also 02294_floating_point_second_in_settings is failing

Algunenano · 2022-05-24T15:07:02Z

@Algunenano 01287_max_execution_speed is failing due to different output also 02294_floating_point_second_in_settings is failing

Thanks a lot for the ping. I have no idea what I did. I remember fixing those issues (and I even wrote about it) but they are broken, so not sure if I dreamed it or discard some changes during the rebase (Or I did it in the other PC and never pushed). I'll have a look and fix the issues. Sorry for the noise.

Update: Yes, I broke it during the rebase (because I switched computers and didn't pull the fixes I had done)

yakov-olkhovskiy · 2022-05-24T15:43:44Z

@Algunenano BTW is this force-push made by github itself? I found that force-push often is very messy...

Algunenano · 2022-05-24T15:56:03Z

BTW is this force-push made by github itself?

No. I did it because the branch was pretty old and rebased instead of merged master/HEAD.

yakov-olkhovskiy · 2022-05-25T20:58:08Z

@Algunenano seems we hit a rare case of CI fluke here:
#37549
we need a new commit to rerun failed build - could you please try force push again?

Algunenano · 2022-05-25T21:08:38Z

@Mergifyio update

mergify · 2022-05-25T21:08:55Z

update

✅ Branch has been successfully updated

Algunenano · 2022-05-25T21:10:10Z

I guess that you need any new commit so I've asked the bot to update the branch. If you actually need a force push let me know and I'll do it tomorrow.

yakov-olkhovskiy · 2022-05-30T19:18:16Z

@mergify update

mergify · 2022-05-30T19:18:35Z

update

✅ Branch has been successfully updated

tavplubix · 2024-01-22T19:23:01Z

tests/queries/0_stateless/02294_decimal_second_errors.sql

+
+-- Ok values
+SELECT 1 SETTINGS max_execution_time=-0.5;
+SELECT 1 SETTINGS max_execution_time=0.5;


DB::Exception: Timeout exceeded: elapsed 0.505827683 seconds, maximum: 0.5. :(
https://s3.amazonaws.com/clickhouse-test-reports/39507/fe1e326701b0566881dc7d51793125a911faa07e/stateless_tests__tsan__[2_5].html

I'll raise it. There are other test already covering 0.something so we don't need to keep a subsecond value here

Algunenano added 2 commits May 13, 2022 11:46

WIP: Seconds as floating point

c330c77

Add tests to check decimal value in seconds

002498b

robot-ch-test-poll2 added the pr-backward-incompatible Pull request with backwards incompatible changes label May 13, 2022

Algunenano added 2 commits May 13, 2022 15:08

Style

6d4bac3

Fix timeout and better comment

d492b1c

Allow negative values for seconds

a2870ef

Algunenano force-pushed the floating_seconds branch from 745965d to a2870ef Compare May 13, 2022 15:28

Algunenano added 2 commits May 13, 2022 20:31

Fix clang tidy error

23dfe49

Fix clang tidy not error

369c18a

Lower the value of settings to match old behaviour

644b7c0

Before the change to use floats, this settings were getting 0 as the parsed value, so keep the old value by setting to zero explicitly

yakov-olkhovskiy self-assigned this May 17, 2022

yakov-olkhovskiy reviewed May 23, 2022

View reviewed changes

Algunenano force-pushed the floating_seconds branch from afdca95 to 4319432 Compare May 23, 2022 22:12

yakov-olkhovskiy approved these changes May 23, 2022

View reviewed changes

Algunenano added 3 commits May 24, 2022 17:10

Merge remote-tracking branch 'blessed/master' into floating_seconds_2

a86aa43

Improve comment

2f37ad7

Reduce flakiness

cd4020f

Algunenano force-pushed the floating_seconds branch from 4319432 to cd4020f Compare May 24, 2022 15:14

Merge branch 'master' into floating_seconds

49cce18

Merge branch 'master' into floating_seconds

b43cfd0

yakov-olkhovskiy approved these changes May 31, 2022

View reviewed changes

yakov-olkhovskiy merged commit c6b20cd into ClickHouse:master May 31, 2022

tavplubix mentioned this pull request Jul 5, 2022

Change in seconds parsing behaviour #38863

Closed

tavplubix reviewed Jan 22, 2024

View reviewed changes

tavplubix mentioned this pull request Jan 22, 2024

Allow to attach partition from table with different partition expression when destination partition expression doesn't re-partition #39507

Merged

Conversation

Algunenano commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Uh oh!

Algunenano commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Algunenano commented May 13, 2022

Uh oh!

Algunenano commented May 13, 2022

Uh oh!

Algunenano commented May 14, 2022

Uh oh!

yakov-olkhovskiy May 23, 2022

Choose a reason for hiding this comment

Uh oh!

Algunenano May 23, 2022

Choose a reason for hiding this comment

Uh oh!

yakov-olkhovskiy May 23, 2022

Choose a reason for hiding this comment

Uh oh!

yakov-olkhovskiy May 23, 2022

Choose a reason for hiding this comment

Uh oh!

Algunenano May 23, 2022

Choose a reason for hiding this comment

Uh oh!

yakov-olkhovskiy commented May 24, 2022

Uh oh!

Algunenano commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yakov-olkhovskiy commented May 24, 2022

Uh oh!

Algunenano commented May 24, 2022

Uh oh!

yakov-olkhovskiy commented May 25, 2022

Uh oh!

Algunenano commented May 25, 2022

Uh oh!

mergify bot commented May 25, 2022

✅ Branch has been successfully updated

Uh oh!

Algunenano commented May 25, 2022

Uh oh!

yakov-olkhovskiy commented May 30, 2022

Uh oh!

mergify bot commented May 30, 2022

✅ Branch has been successfully updated

Uh oh!

tavplubix Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

Algunenano Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Algunenano commented May 13, 2022 •

edited

Loading

Algunenano commented May 13, 2022 •

edited

Loading

Algunenano commented May 24, 2022 •

edited

Loading