Skip to content

Commit bf7f1de

Browse files
jackylee-chclaudeFokkoCopilot
authored
fix(expressions): accept a bare true / false as an AND / OR / NOT operand (#3917)
* fix(expressions): accept a bare true / false as an AND / OR / NOT operand The BooleanLiteral to AlwaysTrue/AlwaysFalse conversion was attached only to the whole expression, so a bare boolean reached And/Or/Not as a BooleanLiteral and failed pydantic validation: parse("(false) or foo = 1") -> EqualTo(foo, 1) parse("false or foo = 1") -> ValidationError: 1 validation error for Or Seeding a filter with `true` and appending clauses is a common way to build a row_filter, so `row_filter="true and status = 'x'"` raised instead of scanning. Give `predicate` its own copy of the boolean element that folds to AlwaysTrue/AlwaysFalse; `literal` and `literal_set` keep the raw BooleanLiteral, so `foo = true` and `foo in (true, false)` are unchanged. Co-Authored-By: Claude Code <noreply@anthropic.com> * Fix parse action for always_boolean to use strtobool Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * refactor(expressions): fold a bare boolean at the predicate level The extra `boolean.copy()` is not needed. `handle_always_expression` already did this fold; it was only attached to the whole expression, which is why `(true) and foo = 1` worked and `true and foo = 1` did not -- infix_notation returns the same Forward that a parenthesized sub-expression recurses through, so the fold ran only when an operand happened to take that path. Attach it to `predicate` instead so every operand folds. `literal` and `literal_set` consume the boolean further in, so `foo = true` and `foo in (true, false)` keep the raw `BooleanLiteral`. The top-level attachment is now unreachable and is removed. Tests are unchanged. Co-Authored-By: Claude Code <noreply@anthropic.com> --------- Co-authored-by: Claude Code <noreply@anthropic.com> Co-authored-by: Fokko Driesprong <fokko@apache.org> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent 9299bdb commit bf7f1de

2 files changed

Lines changed: 39 additions & 23 deletions

File tree

pyiceberg/expressions/parser.py

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,15 @@ def _evaluate_like_statement(result: ParseResults) -> BooleanExpression:
268268
predicate = (between | comparison | in_check | null_check | nan_check | starts_check | boolean).set_results_name("predicate")
269269

270270

271+
@predicate.add_parse_action
272+
def _(result: ParseResults) -> BooleanExpression:
273+
# A bare "true" or "false" in an operand position folds to AlwaysTrue or AlwaysFalse
274+
expr = result[0]
275+
if isinstance(expr, BooleanLiteral):
276+
return AlwaysTrue() if expr.value else AlwaysFalse()
277+
return expr
278+
279+
271280
def handle_not(result: ParseResults) -> Not:
272281
return Not(result[0][0])
273282

@@ -280,29 +289,14 @@ def handle_or(result: ParseResults) -> Or:
280289
return Or(*result[0])
281290

282291

283-
def handle_always_expression(result: ParseResults) -> BooleanExpression:
284-
# If the entire result is "true" or "false", return AlwaysTrue or AlwaysFalse
285-
expr = result[0]
286-
if isinstance(expr, BooleanLiteral):
287-
if expr.value:
288-
return AlwaysTrue()
289-
else:
290-
return AlwaysFalse()
291-
return result[0]
292-
293-
294-
boolean_expression = (
295-
infix_notation(
296-
predicate,
297-
[
298-
(Suppress(NOT), 1, opAssoc.RIGHT, handle_not),
299-
(Suppress(AND), 2, opAssoc.LEFT, handle_and),
300-
(Suppress(OR), 2, opAssoc.LEFT, handle_or),
301-
],
302-
)
303-
.set_name("expr")
304-
.add_parse_action(handle_always_expression)
305-
)
292+
boolean_expression = infix_notation(
293+
predicate,
294+
[
295+
(Suppress(NOT), 1, opAssoc.RIGHT, handle_not),
296+
(Suppress(AND), 2, opAssoc.LEFT, handle_and),
297+
(Suppress(OR), 2, opAssoc.LEFT, handle_or),
298+
],
299+
).set_name("expr")
306300

307301

308302
def parse(expr: str) -> BooleanExpression:

tests/expressions/test_parser.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
AlwaysFalse,
2525
AlwaysTrue,
2626
And,
27+
BooleanExpression,
2728
EqualTo,
2829
GreaterThan,
2930
GreaterThanOrEqual,
@@ -272,3 +273,24 @@ def test_valid_between_with_numerics() -> None:
272273
) == parser.parse("foo between '2025-01-01T00:00:00.000000' and '2025-01-10T12:00:00.000000'")
273274

274275
assert parser.parse("foo between 1 and 3") == parser.parse("1 <= foo and foo <= 3")
276+
277+
278+
@pytest.mark.parametrize(
279+
"expression, expected",
280+
[
281+
("true and foo = 1", EqualTo(Reference("foo"), literal(1))),
282+
("foo = 1 and true", EqualTo(Reference("foo"), literal(1))),
283+
("foo = 1 or false", EqualTo(Reference("foo"), literal(1))),
284+
("foo = 1 or true", AlwaysTrue()),
285+
("foo = 1 and false", AlwaysFalse()),
286+
("not true", AlwaysFalse()),
287+
("not false", AlwaysTrue()),
288+
],
289+
)
290+
def test_boolean_as_operand(expression: str, expected: BooleanExpression) -> None:
291+
assert parser.parse(expression) == expected
292+
293+
294+
def test_boolean_as_literal_is_unchanged() -> None:
295+
assert parser.parse("foo = true") == EqualTo(Reference("foo"), literal(True))
296+
assert parser.parse("foo in (true, false)") == In(Reference("foo"), {literal(True), literal(False)})

0 commit comments

Comments
 (0)