fix(reasoning): support Lakebase/Postgres dialect and rebuild class URIs against the current Base URI - #146
Conversation
…RIs against the current Base URI SQLHelpers.to_number() gains a dialect parameter: on Lakebase (Postgres) it emits a CASE/regex guard instead of TRY_CAST, which does not exist in PostgreSQL. AggregateRuleEngine and DecisionTableEngine now detect the triple-store backend and pass the right dialect through, so numeric comparisons in Aggregate Rules and Decision Tables work against both Databricks SQL and Lakebase. AggregateRuleEngine, DecisionTableEngine, SPARQLRuleEngine and SWRLEngine all reconstruct a rule's target class URI from its local name against the domain's *current* Base URI instead of trusting the class's stored uri field, which goes stale after the domain is re-based (Base URI changed after the class already existed). Without this, a rule silently stopped finding its target class and returned zero results/inferred triples. AggregateRuleEngine also now computes inferred triples whenever a rule has a Result Entity configured, not only when materialize=True — previously, clicking "Run" (preview, no materialize) on such a rule always showed 0 inferred triples even when matching rows existed. DecisionTableEngine additionally ignores an input column left without an associated property (blank mapping) instead of generating a SQL condition that references a table never joined, which raised a "missing FROM-clause entry" error.
|
|
benoitcayladbx
left a comment
benoitcayladbx
left a comment
There was a problem hiding this comment.
Thanks for the production report — all four bugs are real (stale class URI after rebase, TRY_CAST on Lakebase, aggregate preview inferred_count=0, unmapped DT column exploding Postgres). Not mergeable as-is.
Please address the items below (inline comments have the details). Happy to re-review once they land.
Required before merge
-
Scope the class-URI rewrite so imported / foreign vocabularies are not rewritten.
not uri.startswith(base_ns)is not “this class was minted under the old domain Base URI”.owl:Thing,schema:Person, or any OWL import inontology["classes"]would be rewritten to{current_base}{sep}{localName}. Prefix collisions also go the other way (http://example.org/foowill not treathttp://example.org/foobar#Xas stale).Rebuild only when the stored URI is a previous domain base for that class (same local name, namespace equals an old domain URI), not whenever the namespace differs. Extract one helper and use it in all four engines instead of copying the loop.
-
Also rebase a persisted
target_class_uri/result_class_urion the rule._resolve_rule/_resolve_dtstill skip when those fields are already set. If the rule stored the class URI at save time, the uri_map change never runs and the silent 0-results bug remains. -
Do not sniff
type(store).__name__.GraphDBBackend.query_dialectalready exists but Lakebase and Delta both return"sql", which is why the sniff appeared. Add a real backend property (e.g.sql_flavor: "databricks" | "postgres") and a single helper. Class-name matching breaks on wrappers, mocks, and renames. Duplicate_dialect_for_storeon two engines needs to go. -
Keep
sql_cast's postgres branch numeric-only (or type-aware). The regex is applied for everysql_type. Fine while onlysql_numericpassesdialect="postgres"; the nextsql_cast(expr, "STRING"|"TIMESTAMP", dialect="postgres")silently becomes a numeric gate. Split numeric try-cast from generic try-cast. Also note: expr is evaluated twice;"5."fails the regex while DatabricksTRY_CASTwould accept it. -
Do not claim Lakebase numeric support for all four engines.
SWRLBuiltinRegistry(and SPARQL/SHACL emitters) still hardcodeTRY_CAST.greaterThan(?age, 65)on Lakebase will still fail. Either wire SWRL/SPARQL through the same helper, or narrow the PR description to Aggregate + Decision Tables. -
Tests (this is the main process miss). Existing assertions in
tests/units/ontology/test_business_rules.pyandtests/units/core/test_try_cast_policy.pystill expectTRY_CASTand pass viadialect="databricks"without covering the new paths. Please add:_build_uri_map/_resolve_rule: stale domain URI is rebuilt; imported URI is left alone- persisted
target_class_urifrom the old base is rebuilt (item 2) sql_numeric(..., dialect="postgres"): noTRY_CAST, no failing bareCAST, regex-guard presentAggregateRuleEngine.build_sql(..., dialect="postgres")- Decision table: unmapped input column does not appear in FROM/WHERE; if all inputs are unmapped, skip/fail the table instead of matching every instance of the class
- Aggregate dry-run:
inferred_triplesnon-empty whenresult_class_uriis set andmaterialize=False
-
Comments / copy-paste. Repo language is English. Please drop the Spanish comments and the leftover
# (o self._ontology en SWRLEngine)inSPARQLRuleEngine. The→→->message tweak is unrelated — revert unless there is a real encoding issue. -
CLA. The CLA assistant is still pending on this PR; we cannot merge until it is green.
What to keep
- Aggregate preview vs materialize split is correct (
DigitalTwinpersists only onappend_graph/ materialize). - Skipping an unmapped DT join is the right local fix (please just fail-closed when every input is blank).
from back.core.helpers import sql_numericis the right public binding.- The warning log is
%-style— good.
| if dialect == "postgres": | ||
| pg_type = "DOUBLE PRECISION" if sql_type.upper() == "DOUBLE" else sql_type | ||
| return ( | ||
| f"(CASE WHEN ({expr}) ~ '^\\s*[-+]?[0-9]*\\.?[0-9]+([eE][-+]?[0-9]+)?\\s*$' " |
There was a problem hiding this comment.
This postgres branch is a numeric try-cast, but sql_cast is the generic helper. The regex is applied for every sql_type.
Please either:
- keep the regex/
::path only insidesql_numeric, and have postgressql_castfor non-numeric types use a type-appropriate guard (or document thatdialect="postgres"is numeric-only and reject other types), or - make the guard depend on
sql_type.
Also: ({expr}) is evaluated twice (~ then ::). Column refs are fine; don’t copy this pattern onto arbitrary expressions.
"5." fails this regex; Databricks TRY_CAST accepts it. Worth a unit test so the two dialects don’t drift silently.
Add tests in tests/units/core/test_try_cast_policy.py for dialect="postgres" (no TRY_CAST, no statement-aborting bare CAST).
| name = cls.get("name", "") or cls.get("localName", "") | ||
| uri = cls.get("uri", "") | ||
| if not uri and name: | ||
| if base_ns and uri and not uri.startswith(base_ns): |
There was a problem hiding this comment.
not uri.startswith(base_ns) is too broad.
This will rewrite any class whose URI is not under the current domain base — OWL imports, schema.org, owl:Thing, etc. — to {base_uri}{sep}{localName}. That is a behaviour change, not a rebase fix.
startswith is also the wrong namespace test: base http://example.org/foo will not treat http://example.org/foobar#X as stale.
Please extract one helper (used by all four engines) that rebuilds only when:
- the local name matches a domain class, and
- the stored URI’s namespace is a previous domain Base URI (or the class’s own known-stale
uri),
and leaves foreign namespaces untouched.
Related: _resolve below still does if not rule.get(field_uri): return. A rule that persisted target_class_uri / result_class_uri at save time keeps the stale URI and still returns 0 rows. Rebuild those fields too when they fail the same “current base” check.
| def _dialect_for_store(store) -> str: | ||
| """Best-effort SQL dialect detection from the store's class name.""" | ||
| name = type(store).__name__.lower() | ||
| return "postgres" if "lakebase" in name or "postgres" in name else "databricks" |
There was a problem hiding this comment.
Please don’t detect dialect from type(store).__name__. Wrappers, test doubles, and a class rename all silently fall back to Databricks SQL (TRY_CAST) on Lakebase.
GraphDBBackend.query_dialect already exists but Lakebase and Delta both return "sql", so it cannot distinguish them today. Add something like sql_flavor ("databricks" | "postgres") on the backend and read that here.
This method is also duplicated on DecisionTableEngine — one helper, not two copies.
| def _dialect_for_store(store) -> str: | ||
| """Best-effort SQL dialect detection from the store's class name.""" | ||
| name = type(store).__name__.lower() | ||
| return "postgres" if "lakebase" in name or "postgres" in name else "databricks" |
There was a problem hiding this comment.
Same as AggregateRuleEngine._dialect_for_store: class-name sniff + duplication. Please share one backend-driven helper (see comment there).
| name = cls.get("name", "") or cls.get("localName", "") | ||
| uri = cls.get("uri", "") | ||
| if not uri and name: | ||
| if base_ns and uri and not uri.startswith(base_ns): |
There was a problem hiding this comment.
Same class-URI rewrite issue as in AggregateRuleEngine (imports get rewritten). Please call the shared helper instead of copying this loop.
Also: comments in this repo are English — please rewrite this block (and the copies in SPARQL/SWRL) in English.
| i, | ||
| inp.get("property", inp.get("label", "")), | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Skipping an unmapped input is the right fix for missing FROM-clause entry.
Please also fail closed when no input was joined: with joined_aliases empty the query is only rdf:type = target class and every instance matches. Prefer returning None / skipping the table (and keep the warning).
Add a unit test that an unmapped column never appears in FROM/WHERE, and that a table with only unmapped inputs does not match the whole class.
|
|
||
| for cls in ontology.get("classes", []): | ||
| base_ns = base_uri.rstrip("#").rstrip("/") if base_uri else "" | ||
| for cls in ontology.get("classes", []): # (o self._ontology en SWRLEngine) |
There was a problem hiding this comment.
Please drop the leftover # (o self._ontology en SWRLEngine) and the Spanish comment below. Same URI-rewrite helper as the other engines — don’t fork a fourth copy of this loop.
| name = cls.get("name", "") or cls.get("localName", "") | ||
| uri = cls.get("uri", "") | ||
| if not uri and name: | ||
| if base_ns and uri and not uri.startswith(base_ns): |
There was a problem hiding this comment.
Same rewrite / helper request as the other engines.
Note that Lakebase numeric still isn’t covered here: SWRLBuiltinRegistry sql templates keep TRY_CAST. Either route those templates through sql_numeric(dialect=...) or narrow the PR text so we don’t imply SWRL numeric rules work on Postgres after this change.
3 participants
What
Two independent problems in the four reasoning-rule engines (Aggregate Rules, Decision Tables, SPARQL Rules, SWRL), found while running 0.7.1 in production:
1. Rules stop finding their target class after the domain is re-based.
When a domain's Base URI changes after a class already exists, that class can be left with a stale
uri(the old Base URI) in its stored field.AggregateRuleEngine,DecisionTableEngine,SPARQLRuleEngineandSWRLEngineall trusted that storedurias-is to find a rule's target/result class. All four now reconstruct the URI from the class's local name against the domain's current Base URI instead, matching what the rest of the codebase already does in this situation.2. Numeric comparisons fail entirely against Lakebase (Postgres).
SQLHelpers's numeric-cast helper (used by all four engines wherever a rule compares a value numerically — an Aggregate Rule threshold, a Decision Table numeric condition) usedTRY_CAST, which doesn't exist in PostgreSQL. Any such rule failed outright once the graph was stored in Lakebase.SQLHelpers.to_number()now takes adialectparameter; on"postgres"it emits aCASE WHEN <regex validates numeric> THEN CAST … ELSE NULL ENDinstead.Two smaller, engine-specific fixes rode along because they touch the same functions:
AggregateRuleEngine: inferred triples were only computed whenmaterialize=True. An Aggregate Rule with a Result Entity configured but previewed (Run, not materialized) always showed 0 inferred triples even with matching rows. Now computed whenever a Result Entity is configured, consistent with the other rule engines.DecisionTableEngine: an input column left without an associated property (blank mapping) generated a SQL condition referencing a table that was never joined, raisingmissing FROM-clause entry. That column is now skipped instead, and the rest of the row still evaluates.Why
All four are silent-failure modes: a rule that looks correctly configured in the UI either returns nothing or throws a raw SQL error, with no obvious link back to "the domain was renamed" or "the graph lives in Lakebase."
How to test
missing FROM-clause entry; after, that column is ignored and the rest of the table evaluates.Happy to add/adjust automated tests for any of these if that's useful — wanted to get the report and the fix out first rather than block on writing a full suite against your test fixtures.