fix(swrl): builtin atoms in inference/materialization — prefix lookup, datatype-property namespaces, WHERE-filter wiring, Postgres dialect - #172
Conversation
benoitcayladbx
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis in #171 is correct, the four source hunks are the right shape, and the per-defect regression file is exactly the coverage the existing suite was missing. Independently confirmed on current main as well as the v0.8.0 path.
Happy to land this once the items below are addressed. None of them change the approach.
Blockers
- CLA — the assistant bot is still pending. We cannot merge until that’s signed.
scripts/repro_swrl_builtin_defects.py— please drop it from the PR, or rewrite it to take connection info from env / CLI flags. As written it hardcodeshost=localhost port=5433 … password=changeme, which we should not put in a public repo. The unit file (tests/units/ontology/test_swrl_builtin_defects.py) is the piece we want in-tree.- CI — no checks reported on this branch yet. Once CLA + the script cleanup are in, we need the usual unit suite green (
pytest -q -m "not scenario").
Requested changes (keep the fix, tighten the edges)
- Builtin name normalisation —
rsplit(":", 1)[-1]is fine for the editor’sswrlb:greaterThanOrEqual, but it mangles a full IRI (http://www.w3.org/2003/11/swrlb#greaterThanOrEqual). Prefer stripping a known prefix set (swrlb:,swrl:) and/or taking the fragment after#, rather than splitting on the last colon. - Reuse
SWRLParser.partition_rule_atomsinbuild_inference_sql/build_materialization_sqlinstead of hand-rolling the class/prop/builtin split. That duplication is how defect 3 existed while violation SQL was already correct. getattr(self, "_dialect", "databricks")— with__init__setting_dialect, just useself._dialect.- Dialect handling — the post-format
TRY_CAST→CAST/AS DOUBLE→AS DOUBLE PRECISIONmapping is the smallest hammer and is enough for the comparison builtins in the repro. Two notes to either fix or call out explicitly:TRY_CASTvsCASTis a semantic change: Databricks yields NULL on junk literals, Postgres raises and can fail the whole inference run.DATEDIFF,CURRENT_TIMESTAMP(), and some string templates are still Databricks-shaped; date/math builtins can still explode on Lakebase after this patch. Comparison (>=) is what #171 needs; a follow-up for the rest is fine if you document the gap. Longer-term, dialect-specific templates on the builtin descriptors (as you offered) is the cleaner home.
Nits / out of scope (no need to block)
- You’re right that
cypher_templatecurrently holds SQL-ishCAST(… AS DOUBLE), not Cypher — leave it, but thanks for flagging. build_materialization_sqlstill only joins direct properties of the primary var; inference already BFS-walks the chain. Unrelated to #171.
Verified locally that test_greater_than_uses_try_cast can stay green if the Databricks dialect is the default and mapping happens post-format — that part of the design is good.
- builtin name normalisation goes through uri_local_name() (fragment or last
path segment) and then strips the known swrlb:/swrl: prefixes, so full IRIs
resolve; rsplit(':') is gone
- build_inference_sql and build_materialization_sql reuse
SWRLParser.partition_rule_atoms like build_violation_sql; negated atoms now
emit NOT EXISTS instead of being joined as positive patterns
- self._dialect replaces the getattr default at all four call sites
- the Postgres rewrite's semantic and coverage gap (TRY_CAST vs CAST; matches,
dateDiff, now still Databricks-shaped) is documented at the rewrite site
- _build_uri_map writes object properties last so they win a same-name clash,
the same precedence as AggregateRuleEngine
- the repro script is dropped; the unit file carries the regression coverage
- changelog entry added for the changelog-presence gate
|
Thanks for the fast and precise review. Follow-up commit pushed; item by item: Blockers
Requested changes
Also in this commit: the Nits: agreed on both; left Two things noticed while in there, out of scope, no action taken: (a) the |
…efects Editor-generated rules like Sailing(?x) ^ nights(?x, ?n) ^ swrlb:greaterThanOrEqual(?n, 10) -> LongSailing(?x) silently inferred zero triples. Four independent defects, each fatal: 1. SWRLBuiltinRegistry keyed bare lowercase names, but the graphical rule editor emits the standard swrlb: prefix — is_builtin() never matched, degrading the builtin to a phantom predicate join. 2. SWRLEngine._build_uri_map skipped class dataProperties, so datatype atoms resolved to the ontology '#' namespace while the R2RML sync writes predicates under the data '/' namespace. 3. build_inference_sql and build_materialization_sql partitioned atoms by arity only; builtin atoms became triple-pattern joins instead of WHERE filters (build_violation_sql already handled them correctly). 4. Builtin sql_templates are Databricks-dialect (TRY_CAST, bare DOUBLE); the Lakebase (Postgres) backend received the generic translator, so a correct filter was still a syntax error on the default backend. Fixes: prefix-tolerant registry lookup; dataProperties folded into the uri_map; builtin atoms partitioned and emitted as WHERE filters in both builders; a dialect parameter on the translator with Postgres mappings. Repro: scripts/repro_swrl_builtin_defects.py (safe-rule validation, a 3-sailing fixture expecting 2, generated-SQL evidence). Verified: fixture 0->2, and a 2,846-sailing real graph 0->263 matching an independently computed source-of-truth count. Existing suite: 165 reasoning/swrl tests pass unchanged.
- builtin name normalisation goes through uri_local_name() (fragment or last
path segment) and then strips the known swrlb:/swrl: prefixes, so full IRIs
resolve; rsplit(':') is gone
- build_inference_sql and build_materialization_sql reuse
SWRLParser.partition_rule_atoms like build_violation_sql; negated atoms now
emit NOT EXISTS instead of being joined as positive patterns
- self._dialect replaces the getattr default at all four call sites
- the Postgres rewrite's semantic and coverage gap (TRY_CAST vs CAST; matches,
dateDiff, now still Databricks-shaped) is documented at the rewrite site
- _build_uri_map writes object properties last so they win a same-name clash,
the same precedence as AggregateRuleEngine
- the repro script is dropped; the unit file carries the regression coverage
- changelog entry added for the changelog-presence gate
960fa4f to
5415011
Compare
Fixes #171.
Attribute-conditioned SWRL rules silently inferred zero triples. Four independent defects (detailed in the issue); this PR fixes all four with minimal diffs:
SWRLBuiltinRegistry:_normalize()strips an optional namespace prefix soswrlb:greaterThanOrEqual, bare, and any casing all resolve. The editor emits the prefixed form.SWRLEngine._build_uri_map: classdataPropertiesare folded into the property normalisation so datatype predicates resolve to the data namespace exactly like object properties.SWRLSQLTranslator.build_inference_sql/build_materialization_sql: builtin atoms are partitioned out of class/property atoms and emitted as WHERE filters via the existing_build_builtin_filters(mirroringbuild_violation_sql).SWRLSQLTranslator(dialect=...);"postgres"mapsTRY_CAST→CASTandDOUBLE→DOUBLE PRECISION.LakebaseBase.get_query_translator()now passesdialect="postgres".Tests:
tests/units/ontology/test_swrl_builtin_defects.py— one class per defect: prefix-tolerant registry lookup; datatype properties in the uri_map; builtins emitted as WHERE filters (no phantom join) in both the inference and materialization builders; Postgres dialect mapping plus the Lakebase backend handing out a postgres-dialect translator; and a guard that the Databricks dialect is unchanged.scripts/repro_swrl_builtin_defects.py— standalone red/green harness (validates the rule is safe SWRL, builds a 3-row fixture with a known answer, prints the generated SQL, scores inferred-vs-expected) so the defect and fix are independently checkable end to end.Verified: fixture 0→2/2; a real 2,846-instance graph 0→263/263 against an independently computed expected count; the existing suite passes unchanged, including
test_greater_than_uses_try_cast(the dialect mapping happens post-format, so the pinned Databricks templates are untouched).Note on scope:
TRY_CAST-style dialect handling could arguably live in the builtin descriptors rather than a post-hoc string mapping; we kept the change minimal and localized. Happy to rework to maintainer preference. Thecypher_templatefield currently holds SQL-flavoured text rather than Cypher — left untouched, but flagging it.