From 7cd1bafabcfcf1fa07b0734e1978f26a06f7d69b Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 23 Apr 2026 02:32:56 -0700 Subject: [PATCH] address review: check has_implicit_cancel return value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit diagnose() in lib.py calls spec.has_implicit_cancel() and uses the return value; the extractor was treating any override as cancel support, regardless of what it returns. An override that explicitly returns False (e.g. ImpalaEngineSpec) should NOT enable query_cancelation — only a cancel_query override should, per has_custom_method(spec, "cancel_query") or spec.has_implicit_cancel(). Added static_return_bool() to statically resolve simple "return " methods. has_implicit_cancel overrides that explicitly return False are now excluded from direct_methods; True / unresolvable / complex bodies still count (conservative). For current specs the outcome is unchanged (Impala/Hive have cancel_query overrides; Presto/Hive return True), but the heuristic now matches diagnose() exactly and won't misclassify future specs that override has_implicit_cancel to False without a cancel_query override. Co-Authored-By: Claude Opus 4.7 --- docs/scripts/generate-database-docs.mjs | 45 +++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/docs/scripts/generate-database-docs.mjs b/docs/scripts/generate-database-docs.mjs index b02d7d42ad4..2af06861c18 100644 --- a/docs/scripts/generate-database-docs.mjs +++ b/docs/scripts/generate-database-docs.mjs @@ -141,6 +141,42 @@ def eval_node(node): return "" return None +def static_return_bool(func_node): + """ + Statically resolve a method's return value to a bool when possible. + + Returns True/False for functions whose body is (effectively) a single + \`return True\` / \`return False\` — allowing a leading docstring and + ignoring pure-comment/pass statements. Returns None for anything more + complex (conditional returns, computed values, no return, etc.). + + Used by \`has_implicit_cancel\` handling: \`diagnose()\` in lib.py calls + the method and checks the return value, so an override that explicitly + returns False must NOT be treated as enabling query cancelation. + """ + returns = [] + other_logic = False + for stmt in func_node.body: + # Skip docstring (first expression statement that's a string constant) + if (isinstance(stmt, ast.Expr) + and isinstance(stmt.value, ast.Constant) + and isinstance(stmt.value.value, str)): + continue + if isinstance(stmt, ast.Pass): + continue + if isinstance(stmt, ast.Return): + returns.append(stmt) + continue + # Any other statement (if/for/assign/etc.) means control flow is + # non-trivial; bail out to be conservative. + other_logic = True + break + if other_logic or len(returns) != 1: + return None + val = eval_node(returns[0].value) + return val if isinstance(val, bool) else None + + def deep_merge(base, override): """Deep merge two dictionaries. Override values take precedence.""" if base is None: @@ -299,6 +335,15 @@ for filename in sorted(os.listdir(specs_dir)): unresolved_cap_attrs.add(target.id) elif isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)): if item.name in CAP_METHODS: + # has_implicit_cancel is special: diagnose() uses the + # method's RETURN VALUE, not just its presence. If the + # override statically returns False, treat it as if + # the method weren't overridden so query_cancelation + # matches diagnose(). Unresolvable / True / anything + # else falls through as an override (conservative). + if item.name == 'has_implicit_cancel': + if static_return_bool(item) is False: + continue direct_methods.add(item.name) # Check for engine attribute with non-empty value to distinguish