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 applying settings for FORMAT on the client #46003

Merged
merged 2 commits into from
Feb 8, 2023

Conversation

azat
Copy link
Collaborator

@azat azat commented Feb 3, 2023

Changelog category (leave one):

  • Backward Incompatible Change

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

Fix applying settings for FORMAT on the client

Previously the following query does not works correctly:

SELECT number FROM numbers(5) SETTINGS output_format_json_array_of_rows = 1 FORMAT JSONEachRow

While this one works OK:

SELECT number FROM numbers(5) FORMAT JSONEachRow SETTINGS output_format_json_array_of_rows = 1

The problem is in which AST those settings are stored, use the logic as executeQuery() has to apply them:

/// Interpret SETTINGS clauses as early as possible (before invoking the corresponding interpreter),
/// to allow settings to take effect.
if (const auto * select_query = ast->as<ASTSelectQuery>())
{
if (auto new_settings = select_query->settings())
InterpreterSetQuery(new_settings, context).executeForCurrentContext();
}
else if (const auto * select_with_union_query = ast->as<ASTSelectWithUnionQuery>())
{
applySettingsFromSelectWithUnion(*select_with_union_query, context);
}
else if (const auto * query_with_output = dynamic_cast<const ASTQueryWithOutput *>(ast.get()))
{
if (query_with_output->settings_ast)
InterpreterSetQuery(query_with_output->settings_ast, context).executeForCurrentContext();
if (const auto * create_query = ast->as<ASTCreateQuery>())
{
if (create_query->select)
{
applySettingsFromSelectWithUnion(create_query->select->as<ASTSelectWithUnionQuery &>(), context);
}
}
}
else if (auto * insert_query = ast->as<ASTInsertQuery>())
{
context->setInsertFormat(insert_query->format);
if (insert_query->settings_ast)
InterpreterSetQuery(insert_query->settings_ast, context).executeForCurrentContext();
insert_query->tail = istr;
}

Note, the only problem should be with the settings for FORMAT, since client applies thoes settings (and formats) locally w/o server, while in case of i.e. HTTP it will be applied on the server and everything will works fine.

Refs: #45880 (cc @tavplubix @m1khal3v)
Follow-up for: #12480

@azat
Copy link
Collaborator Author

azat commented Feb 4, 2023

And now you can see the "reason" why it was written that way, after this patch the following query changes their behavior:

$ echo 'Custom true' | clickhouse-local -q "SELECT * FROM file('/dev/stdin', 'TSV', 'bool Bool') settings bool_true_representation='Custom true'"
Custom true

Before this PR clickhouse-client/clickhouse-local does not apply SETTINGS clause on the client side, but server does applies it, and so the server correctly parse Custom true as Bool(true), and so client outputs true

But, with this PR now client interpret bool_true_representation setting too, and that's why it outputs Custom true instead of true because this setting applies for both sides: deserialization and serialization.

Also here are few other variants without this patch, that may looks odd:

$ echo 'Custom true' | ch local -q "SELECT * FROM file('/dev/stdin', 'TSV', 'bool Bool') format TSV settings bool_true_representation='Custom true'"
Custom true

$ echo 'Custom true' | ch local -q "SELECT * FROM file('/dev/stdin', 'TSV', 'bool Bool')" --bool_true_representation='Custom true'
Custom true

@azat
Copy link
Collaborator Author

azat commented Feb 4, 2023

@tavplubix @Avogar or maybe @alexey-milovidov maybe you have some thoughts on this?

To me it looks odd that SETTINGS in one place, works differently.
Maybe there should be some setting like format_settings_context='none|both|serialization|deserilization' (or input/output)?

@tavplubix tavplubix self-assigned this Feb 6, 2023
Copy link
Member

@tavplubix tavplubix left a comment

Choose a reason for hiding this comment

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

I like the new behavior more than the old one, thank you!

@tavplubix
Copy link
Member

I think we can simply update the tests

Previously the following query does not works correctly:

    SELECT number FROM numbers(5) SETTINGS output_format_json_array_of_rows = 1 FORMAT JSONEachRow

While this one works OK:

    SELECT number FROM numbers(5) FORMAT JSONEachRow SETTINGS output_format_json_array_of_rows = 1

The problem is in which AST those settings are stored, use the logic as
executeQuery() has to apply them:

  https://github.com/ClickHouse/ClickHouse/blob/c83f701696451c5bbd856d5f5503ba1553388e5d/src/Interpreters/executeQuery.cpp#L467-L497

Note, the only problem should be with the settings for FORMAT, since
client applies thoes settings (and formats) locally w/o server, while in
case of i.e. HTTP it will be applied on the server and everything will
works fine.

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
@robot-ch-test-poll3 robot-ch-test-poll3 added pr-backward-incompatible Pull request with backwards incompatible changes and removed pr-bugfix Pull request with bugfix, not backported by default labels Feb 7, 2023
@azat
Copy link
Collaborator Author

azat commented Feb 7, 2023

I think we can simply update the tests

I am a bit worried about serialization vs deserialization. But OK, let's try.

Refernce files had been checked manually and using this onelinear:

  $ diff <(jq -r .bool ../tests/queries/0_stateless/02152_bool_type_parsing.stdout) ../tests/queries/0_stateless/02152_bool_type_parsing.reference

Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
@tavplubix
Copy link
Member

Integration tests (asan) [3/6] - #45437

@tavplubix tavplubix merged commit 648812b into ClickHouse:master Feb 8, 2023
@azat azat deleted the client-settings-fix branch February 9, 2023 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-backward-incompatible Pull request with backwards incompatible changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants