Skip to content

01674_executable_dictionary_implicit_key: executable_dictionary: Use printf#26978

Merged
kitaisreal merged 3 commits intoClickHouse:masterfrom
Algunenano:improve_executable_dictionary
Aug 2, 2021
Merged

01674_executable_dictionary_implicit_key: executable_dictionary: Use printf#26978
kitaisreal merged 3 commits intoClickHouse:masterfrom
Algunenano:improve_executable_dictionary

Conversation

@Algunenano
Copy link
Copy Markdown
Member

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

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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

Detailed description / Documentation draft:

01674_executable_dictionary_implicit_key fails because it can't parse the output of the dictionary:

2021.07.29 18:24:50.718247 [ 771410 ] {} <Error> void DB::ParallelParsingInputFormat::onBackgroundException(size_t): Code: 27. DB::ParsingException: Cannot parse input: expected '\t' before: '\\tFirstKey\\tValue\n': 
Row 1:
Column 0,   name: id,     type: UInt64, parsed text: "1"
ERROR: garbage after UInt64: "<BACKSLASH>tFirstKey"

The problem is that echo requires -e to recognize scape sequences, for example:

$ echo "1\tFirstKey\tValue"
1\tFirstKey\tValue

$ echo -e "1\tFirstKey\tValue"
1       FirstKey        Value

I've tested this with GNU coreutils 8.32 with bash (Linux) and with zsh 5.8 (OSX 11.3.1) and they work the same way. Another option would be to replace echo and use printf instead.

@robot-clickhouse robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jul 29, 2021
@qoega
Copy link
Copy Markdown
Member

qoega commented Jul 29, 2021

Test fails in docker. We use Ubuntu there

': Code: 510. DB::Exception: Update failed for dictionary simple_executable_cache_dictionary_no_implicit_key : Code: 27. DB::ParsingException: Cannot parse input: expected '\t' before: '-e 1\tValue\n': 
2021-07-29 20:01:49 Row 1:
2021-07-29 20:01:49 Column 0,   name: id,    type: UInt64, ERROR: text "-e 1<TAB>Value" is not like UInt64
2021-07-29 20:01:49 
2021-07-29 20:01:49 : (at row 1)
2021-07-29 20:01:49 . (CANNOT_PARSE_INPUT_ASSERTION_FAILED),. (CACHE_DICTIONARY_UPDATE_FAIL)
2021-07-29 20:01:49 , result:
2021-07-29 20:01:49 
2021-07-29 20:01:49 
2021-07-29 20:01:49 Database: test_xh3gtb

@Algunenano
Copy link
Copy Markdown
Member Author

Test fails in docker. We use Ubuntu there

Let's see if printf works in both places.

@kitaisreal kitaisreal self-assigned this Jul 29, 2021
@Algunenano Algunenano changed the title 01674_executable_dictionary_implicit_key: executable_dictionary: Use echo -e 01674_executable_dictionary_implicit_key: executable_dictionary: Use printf Jul 29, 2021
@alexey-milovidov
Copy link
Copy Markdown
Member

2021-07-29 20:32:52 00705_drop_create_merge_tree: [ FAIL ] - having stderror:
2021-07-29 20:32:52 Received exception from server (version 21.9.1):
2021-07-29 20:32:52 Code: 219. DB::Exception: Received from localhost:9000. DB::Exception: New table appeared in database being dropped or detached. Try again.. (DATABASE_NOT_EMPTY)

Looks like a normal situation, need to filter these messages in test?

@Algunenano
Copy link
Copy Markdown
Member Author

00537_quarters fails because of the timezone (America/Asuncion). I'll have a look

@Algunenano
Copy link
Copy Markdown
Member Author

2021-07-29 20:32:52 00705_drop_create_merge_tree: [ FAIL ] - having stderror:
2021-07-29 20:32:52 Received exception from server (version 21.9.1):
2021-07-29 20:32:52 Code: 219. DB::Exception: Received from localhost:9000. DB::Exception: New table appeared in database being dropped or detached. Try again.. (DATABASE_NOT_EMPTY)

Looks like a normal situation, need to filter these messages in test?

Looking at the test, it seems like it could be a bug in the test. The test is waiting for all subprocesses to finish and when the timeout is reached the process will

but when a timeout is reached the process will kill the client, but that doesn't inmediately stops the query that was running in the server. If I'm not wrong it could happen that:

  • Client starts query to create the table (reaches the server)
  • Timeout happens, the client is killed.
  • The test receives the notification that all 5 subprocesses are done.
  • The tests drops the table (since it doesn't exist yet it won't do anything).
  • The test ends and drops the database.
  • While the database is being dropped, the initial client request to creates the table finishes.
  • The error is triggered.

I haven't been able to reproduce it locally (even by adding sleeps) but it's something that I see possible. If all that is true, then I think the best idea is to use the signal as a way to break the loop, instead of a direct TERM or KILL. I'll look into it.

@alexey-milovidov
Copy link
Copy Markdown
Member

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jul 30, 2021

Command update: success

Branch has been successfully updated

@Algunenano
Copy link
Copy Markdown
Member Author

The failing test (00719_format_datetime_rand) seems like a true bug:

WITH toDateTime('1970-06-17 07:39:21', 'Africa/Monrovia') AS t
SELECT
    toUnixTimestamp(t),
    timeZoneOffset(t),
    formatDateTime(t, '%F %T', 'Africa/Monrovia'),
    toString(t, 'Africa/Monrovia')

Query id: e8484df3-58cc-4fc8-8fd1-9c3660425585

┌─toUnixTimestamp(t)─┬─timeZoneOffset(t)─┬─formatDateTime(t, '%F %T', 'Africa/Monrovia')─┬─toString(t, 'Africa/Monrovia')─┐
│           14459031-26701970-06-17 07:39:511970-06-17 07:39:21            │
└────────────────────┴───────────────────┴───────────────────────────────────────────────┴────────────────────────────────┘

There is a 30 second diff between formatDateTime and toString. toString is correct according to https://www.epochconverter.com/timezones?q=14459031&tz=Africa%2FMonrovia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants