diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e7bbd77e5..557df1fa6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed +- **GFQL Cypher conversion operand validation**: statically invalid conversion operands, such as lists, maps, and graph entities, are rejected at compile time before the engine sees them. Dynamic operands, null propagation, and list-comprehension lexical shadowing keep their existing behavior. + - **GFQL remote Let/DAG compatibility**: `gfql_remote` no longer substitutes an empty `gfql_operations` array for a Let/DAG query, so unsupported older servers receive `gfql_query` and reject it explicitly instead of silently running an identity program. - **GFQL temporal scalar filters**: an empty timestamp or duration string no longer silently becomes a `NaT` bound; it raises the same typed `GFQLSchemaError` E302 as an unparseable bound. Naive bounds are handled as `Timestamp` values before timezone localization, and optional `pytz` exception discovery is confined to aware-column localization. - **GFQL Cypher null-equality conjunctions**: comparisons such as `n.v = null` now preserve three-valued `UNKNOWN` when combined with `AND`, instead of being replaced as if no predicate existed. `NeverMatch` predicates now round-trip through GFQL JSON, and raw JSON `filter_dict` null matching is unchanged. diff --git a/graphistry/compute/gfql/cypher/lowering.py b/graphistry/compute/gfql/cypher/lowering.py index 0d02cf9440..3f8c371b7d 100644 --- a/graphistry/compute/gfql/cypher/lowering.py +++ b/graphistry/compute/gfql/cypher/lowering.py @@ -97,6 +97,7 @@ _rebuild_expr_node, collect_identifiers, parse_expr, + iter_expr_children, walk_expr_nodes, is_expr_node, ) @@ -1009,40 +1010,29 @@ def _raise_invalid(op_name: str) -> None: ) -def _list_item_invalid_for_numeric_conversion( - item: ExprNode, +def _conversion_operand_statically_invalid( + function_name: str, + operand: ExprNode, *, alias_targets: Optional[Mapping[str, ASTObject]], - allow_boolean: bool, + ignored_aliases: AbstractSet[str] = frozenset(), + invalid_aliases: AbstractSet[Tuple[str, str]] = frozenset(), ) -> bool: - if isinstance(item, ExprLiteral): - if isinstance(item.value, (list, dict)): - return True - return isinstance(item.value, bool) and not allow_boolean - if isinstance(item, (ListLiteral, MapLiteral)): + name = function_name.lower() + if isinstance(operand, (ListLiteral, MapLiteral)): return True - if isinstance(item, Identifier): - return alias_targets is not None and item.name in alias_targets - if isinstance(item, FunctionCall): - return item.name in {"__node_entity__", "__edge_entity__"} + if isinstance(operand, ExprLiteral): + return isinstance(operand.value, (list, dict)) or (name == "tofloat" and isinstance(operand.value, bool)) + if isinstance(operand, Identifier): + if (operand.name, name) in invalid_aliases: + return True + return alias_targets is not None and operand.name in alias_targets and operand.name not in ignored_aliases + if isinstance(operand, FunctionCall): + return operand.name in {"__node_entity__", "__edge_entity__"} return False -def _list_item_invalid_for_tostring( - item: ExprNode, - *, - alias_targets: Optional[Mapping[str, ASTObject]], -) -> bool: - if isinstance(item, ExprLiteral): - return isinstance(item.value, (list, dict)) - if isinstance(item, (ListLiteral, MapLiteral)): - return True - if isinstance(item, Identifier): - return alias_targets is not None and item.name in alias_targets - if isinstance(item, FunctionCall): - return item.name in {"__node_entity__", "__edge_entity__"} - return False - +_CYPHER_CONVERSION_FUNCTIONS = frozenset({"tointeger", "tofloat", "toboolean", "tostring"}) def _contains_aggregate_call(node: ExprNode) -> bool: found = False @@ -1101,34 +1091,44 @@ def _enter(current: ExprNode) -> None: _raise("Cypher list comprehensions cannot contain aggregate functions in the local compiler") if current.projection is not None and _contains_aggregate_call(current.projection): _raise("Cypher list comprehensions cannot contain aggregate functions in the local compiler") - if ( - isinstance(current.projection, FunctionCall) - and len(current.projection.args) == 1 - and isinstance(current.projection.args[0], Identifier) - and current.projection.args[0].name == current.var - and isinstance(current.source, ListLiteral) - ): - if current.projection.name.lower() in {"tofloat", "tointeger"}: + walk_expr_nodes(node, enter=_enter) + + def _validate_conversion_scope(current: ExprNode, bound_aliases: AbstractSet[str], invalid_aliases: AbstractSet[Tuple[str, str]]) -> None: + if ( + isinstance(current, FunctionCall) + and current.name.lower() in _CYPHER_CONVERSION_FUNCTIONS + and len(current.args) == 1 + and _conversion_operand_statically_invalid( + current.name, + current.args[0], + alias_targets=alias_targets, + ignored_aliases=bound_aliases, + invalid_aliases=invalid_aliases, + ) + ): + _raise(f"Cypher {current.name}() cannot consume the statically invalid operand type") + if isinstance(current, (ListComprehension, QuantifierExpr)): + _validate_conversion_scope(current.source, bound_aliases, invalid_aliases) + nested_aliases = bound_aliases | {current.var} + nested_invalid_aliases = {(alias, function) for alias, function in invalid_aliases if alias != current.var} + if isinstance(current, ListComprehension) and isinstance(current.source, ListLiteral): + nested_invalid_aliases |= { + (current.var, function_name) + for function_name in _CYPHER_CONVERSION_FUNCTIONS if any( - _list_item_invalid_for_numeric_conversion( - item, - alias_targets=alias_targets, - allow_boolean=current.projection.name.lower() == "tointeger", + _conversion_operand_statically_invalid( + function_name, item, alias_targets=alias_targets, ignored_aliases=bound_aliases ) for item in current.source.items - ): - _raise( - "Cypher toFloat()/toInteger() cannot consume the current " - "list-comprehension item types in the local compiler" - ) - if current.projection.name == "tostring": - if any( - _list_item_invalid_for_tostring(item, alias_targets=alias_targets) - for item in current.source.items - ): - _raise("Cypher toString() cannot consume the current list-comprehension item types in the local compiler") + ) + } + for child in (current.predicate,) if isinstance(current, QuantifierExpr) else cast(Tuple[ExprNode, ...], tuple(filter(None, (current.predicate, current.projection)))): + _validate_conversion_scope(child, nested_aliases, nested_invalid_aliases) + else: + for child in iter_expr_children(current): + _validate_conversion_scope(child, bound_aliases, invalid_aliases) - walk_expr_nodes(node, enter=_enter) + _validate_conversion_scope(node, frozenset(), frozenset()) def _validate_with_projection_aliasing(stage: ProjectionStage) -> None: diff --git a/graphistry/tests/compute/gfql/test_unary_op_surface.py b/graphistry/tests/compute/gfql/test_unary_op_surface.py index 28f65b6ff7..8aedb3c509 100644 --- a/graphistry/tests/compute/gfql/test_unary_op_surface.py +++ b/graphistry/tests/compute/gfql/test_unary_op_surface.py @@ -564,3 +564,118 @@ def test_numeric_comprehension_invalid_consumed_items_still_raise(engine): with pytest.raises(GFQLValidationError) as error: _graph(engine).gfql(f"MATCH (n) RETURN {expression} AS v", engine=engine) assert error.value.code == ErrorCode.E108 + + +@pytest.mark.parametrize("expression", [ + "toInteger([])", + "toInteger({a: 1})", + "toFloat([])", + "toFloat({a: 1})", + "toFloat(true)", + "toBoolean([])", + "toBoolean({a: 1})", + "toString([])", + "toString({a: 1})", +]) +def test_conversion_rejects_statically_invalid_direct_literals(expression): + """Validation happens before an engine sees a provably unsupported conversion.""" + with pytest.raises(GFQLValidationError) as error: + graphistry.nodes(pd.DataFrame({"id": ["a"]})).gfql( + f"MATCH (n) RETURN {expression} AS v", language="cypher" + ) + assert error.value.code == ErrorCode.E108 + + +@pytest.mark.parametrize("function", ["toInteger", "toFloat", "toBoolean", "toString"]) +def test_conversion_rejects_statically_invalid_bound_parameters(function): + """Compile-time parameter folding participates in the same static type check.""" + with pytest.raises(GFQLValidationError) as error: + graphistry.nodes(pd.DataFrame({"id": ["a"]})).gfql( + f"MATCH (n) RETURN {function}($value) AS v", + params={"value": []}, + language="cypher", + ) + assert error.value.code == ErrorCode.E108 + + +@pytest.mark.parametrize("engine", ENGINES) +@pytest.mark.parametrize("expression,expected", [ + ("toInteger('41')", 41), + ("toFloat(2)", 2.0), + ("toBoolean(true)", True), + ("toString(2)", "2"), +]) +def test_conversion_valid_literal_executes_across_engines(engine, expression, expected): + """Each conversion keeps at least one statically valid scalar execution path.""" + assert _values(engine, expression) == [expected] * 4 + + +@pytest.mark.parametrize("engine", ["pandas"]) +def test_conversion_unconsumed_invalid_comprehension_items_compile(engine): + """The source item is not an operand when the projection ignores it.""" + assert _values(engine, "[x IN [1, []] | toString('x')]") == [["x", "x"]] * 4 + + +@pytest.mark.parametrize("engine", ["pandas"]) +@pytest.mark.parametrize("expression,expected", [ + ("[n IN [1] | toInteger(n)]", [[1]]), + ("[n IN ['x'] | toString(n)]", [["x"]]), + ("[x IN [1, []] | [x IN [1] | toInteger(x)]]", [[[1], [1]]]), +]) +def test_conversion_respects_comprehension_lexical_binding(engine, expression, expected): + """Comprehension variables shadow graph aliases; graph aliases remain rejected.""" + assert _values(engine, expression) == expected * 4 + with pytest.raises(GFQLValidationError) as error: + _graph(engine).gfql( + "MATCH (n) RETURN [x IN [1] | toInteger(n)] AS v", engine=engine + ) + assert error.value.code == ErrorCode.E108 + + +@pytest.mark.parametrize("engine", ["pandas"]) +def test_conversion_ignores_shadowed_outer_item_in_nested_argument(engine): + """An inner comprehension binding hides the outer item, even in nested arguments.""" + expression = "[x IN [1, []] | toString([x IN [1] | x])]" + assert _values(engine, expression) == [["[1]", "[1]"]] * 4 + + +@pytest.mark.parametrize("engine", ["pandas"]) +def test_conversion_uses_outer_scope_for_nested_source(engine): + """A nested source is evaluated before its binding shadows the outer item.""" + for variable in ["x", "y"]: + expression = f"[x IN [1, []] | [{variable} IN [toInteger(x)] | {variable}]]" + with pytest.raises(GFQLValidationError) as error: + _graph(engine).gfql(f"MATCH (n) RETURN {expression} AS v", engine=engine) + assert error.value.code == ErrorCode.E108 + + +@pytest.mark.parametrize("engine", ["pandas"]) +def test_conversion_uses_outer_scope_for_nested_quantifier_source(engine): + """A quantifier source is also evaluated before its binding is introduced.""" + for variable in ["x", "y"]: + expression = f"[x IN [1, []] | any({variable} IN [toInteger(x)] WHERE {variable})]" + with pytest.raises(GFQLValidationError) as error: + _graph(engine).gfql(f"MATCH (n) RETURN {expression} AS v", engine=engine) + assert error.value.code == ErrorCode.E108 + + +@pytest.mark.parametrize("engine", ["pandas"]) +@pytest.mark.parametrize("source", ["[1]", "['1']"]) +def test_conversion_shadowed_source_still_runs_valid_body(engine, source): + """Binding a valid nested source preserves the shadowed comprehension body.""" + assert _values(engine, f"[x IN [1, []] | [x IN {source} | toInteger(x)]]") == [[[1], [1]]] * 4 + + +@pytest.mark.parametrize("engine", ["pandas"]) +@pytest.mark.parametrize("function,expected", [ + ("toInteger", 1), + ("toFloat", 1.0), + ("toBoolean", True), + ("toString", "1"), +]) +def test_conversion_binding_is_lexical_for_nested_variables(engine, function, expected): + """A differently named nested binding leaves the outer comprehension item visible.""" + expression = f"[x IN [1, []] | [y IN ['1'] | {function}(x)]]" + with pytest.raises(GFQLValidationError) as error: + _graph(engine).gfql(f"MATCH (n) RETURN {expression} AS v", engine=engine) + assert error.value.code == ErrorCode.E108