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 formatting of NOT with single literals #60042

Merged
merged 2 commits into from Feb 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 12 additions & 9 deletions src/Parsers/ASTFunction.cpp
Expand Up @@ -835,34 +835,37 @@ void ASTFunction::formatImplWithoutAlias(const FormatSettings & settings, Format

const auto * literal = arguments->children[0]->as<ASTLiteral>();
const auto * function = arguments->children[0]->as<ASTFunction>();
bool negate = name == "negate";
bool is_tuple = literal && literal->value.getType() == Field::Types::Tuple;
// do not add parentheses for tuple literal, otherwise extra parens will be added `-((3, 7, 3), 1)` -> `-(((3, 7, 3), 1))`
bool literal_need_parens = literal && !is_tuple;

// negate always requires parentheses, otherwise -(-1) will be printed as --1
bool negate_need_parens = negate && (literal_need_parens || (function && function->name == "negate"));
// We don't need parentheses around a single literal.
bool need_parens = !literal && frame.need_parens && !negate_need_parens;
bool inside_parens = name == "negate" && (literal_need_parens || (function && function->name == "negate"));

/// We DO need parentheses around a single literal
/// For example, SELECT (NOT 0) + (NOT 0) cannot be transformed into SELECT NOT 0 + NOT 0, since
/// this is equal to SELECT NOT (0 + NOT 0)
bool outside_parens = frame.need_parens && !inside_parens;

// do not add extra parentheses for functions inside negate, i.e. -(-toUInt64(-(1)))
if (negate_need_parens)
if (inside_parens)
nested_need_parens.need_parens = false;

if (need_parens)
if (outside_parens)
settings.ostr << '(';

settings.ostr << (settings.hilite ? hilite_operator : "") << func[1] << (settings.hilite ? hilite_none : "");

if (negate_need_parens)
if (inside_parens)
settings.ostr << '(';

arguments->formatImpl(settings, state, nested_need_parens);
written = true;

if (negate_need_parens)
if (inside_parens)
settings.ostr << ')';

if (need_parens)
if (outside_parens)
settings.ostr << ')';

break;
Expand Down
4 changes: 2 additions & 2 deletions tests/queries/0_stateless/01920_not_chain_format.reference
@@ -1,5 +1,5 @@
-- { echo }
EXPLAIN SYNTAX SELECT NOT NOT (NOT (NOT (NULL)));
SELECT NOT (NOT (NOT NOT NULL))
SELECT NOT (NOT (NOT (NOT NULL)))
EXPLAIN SYNTAX SELECT NOT (NOT (NOT NOT NULL));
SELECT NOT (NOT (NOT NOT NULL))
SELECT NOT (NOT (NOT (NOT NULL)))
4 changes: 2 additions & 2 deletions tests/queries/0_stateless/01921_not_chain.reference
Expand Up @@ -4,6 +4,6 @@ SELECT 1 != (NOT 1);
SELECT 1 != NOT 1;
1
EXPLAIN SYNTAX SELECT 1 != (NOT 1);
SELECT 1 != NOT 1
SELECT 1 != (NOT 1)
EXPLAIN SYNTAX SELECT 1 != NOT 1;
SELECT 1 != NOT 1
SELECT 1 != (NOT 1)
13 changes: 13 additions & 0 deletions tests/queries/0_stateless/02990_format_not_precedence.reference
@@ -0,0 +1,13 @@
-- { echoOn }
SELECT NOT 0 + NOT 0;
0
SELECT NOT (0 + (NOT 0));
0
SELECT (NOT 0) + (NOT 0);
2
SELECT formatQuery('SELECT NOT 0 + NOT 0');
SELECT NOT (0 + (NOT 0))
SELECT formatQuery('SELECT NOT (0 + (NOT 0))');
SELECT NOT (0 + (NOT 0))
SELECT formatQuery('SELECT (NOT 0) + (NOT 0)');
SELECT (NOT 0) + (NOT 0)
7 changes: 7 additions & 0 deletions tests/queries/0_stateless/02990_format_not_precedence.sql
@@ -0,0 +1,7 @@
-- { echoOn }
SELECT NOT 0 + NOT 0;
SELECT NOT (0 + (NOT 0));
SELECT (NOT 0) + (NOT 0);
SELECT formatQuery('SELECT NOT 0 + NOT 0');
SELECT formatQuery('SELECT NOT (0 + (NOT 0))');
SELECT formatQuery('SELECT (NOT 0) + (NOT 0)');