#228 - Support reading custom properties from SDF files during indexing in Bingo Elastic

.claude/claude-docs/bingo-elastic-python.md

@@ -4,10 +4,10 @@ The standalone Python library at `bingo/bingo-elastic/python/` that indexes Indi
 
 ## Module map
 
-- `bingo_elastic/elastic.py` — `ElasticRepository` and `AsyncElasticRepository` (parallel sync/async classes, both take a `tau_search: bool` flag), `IndexName` enum (`BINGO_MOLECULE`, `BINGO_REACTION`, `BINGO_CUSTOM`), `build_index_body(tau_search)` builder for the index mapping, and `compile_query` (dispatches a query subject + kwargs to the right query class; reroutes substructure to the tautomer path when `options` contains `TAU`).
+- `bingo_elastic/elastic.py` — `ElasticRepository` and `AsyncElasticRepository` (parallel sync/async classes, both take `tau_search: bool` and `custom_properties: CustomPropertiesMapping` flags), `IndexName` enum (`BINGO_MOLECULE`, `BINGO_REACTION`, `BINGO_CUSTOM`), `build_index_body(tau_search, custom_properties)` builder for the index mapping (merges `custom_properties` into `mappings.properties` and rejects collisions with `RESERVED_FIELDS`), and `compile_query` (dispatches a query subject + kwargs to the right query class; reroutes substructure to the tautomer path when `options` contains `TAU`). `CustomPropertiesMapping = Dict[str, Dict[str, Any]]`: keys are field names, values are ES property-mapping fragments (e.g. `{"n": {"type": "integer"}}`).
 - `bingo_elastic/queries.py` — `CompilableQuery` hierarchy: `SubstructureQuery`, `TautomerSubstructureQuery` (subclass swapping in the `sub-tau` fingerprint and `tau_fingerprint` field), `ExactMatch`, similarity matches (`TanimotoSimilarityMatch`, `EuclidSimilarityMatch`, `TverskySimilarityMatch`), plus `KeywordQuery`, `RangeQuery`, `WildcardQuery` for non-chemical fields. `query_factory` maps kwarg keys (`"substructure"`, `"tautomer"`, `"exact"`, …) to a class.
-- `bingo_elastic/model/record.py` — `IndigoRecord` (abstract), `IndigoRecordMolecule`, `IndigoRecordReaction`, and the `WithIndigoObject` descriptor that extracts fingerprints + `cmf` + `hash` from an `IndigoObject` at construction time. The descriptor also computes the `sub-tau` fingerprint when the record was built with `tau_search=True`.
-- `bingo_elastic/model/helpers.py` — file iterators (`iterate_file`, `load_reaction`).
+- `bingo_elastic/model/record.py` — `IndigoRecord` (abstract), `IndigoRecordMolecule`, `IndigoRecordReaction`, and the `WithIndigoObject` descriptor that extracts fingerprints + `cmf` + `hash` from an `IndigoObject` at construction time. The descriptor also computes the `sub-tau` fingerprint when the record was built with `tau_search=True`, and copies non-reserved properties from `iterateProperties()` (SDF tags) onto the record. `IndigoRecord(custom_properties=…)` accepts an iterable of property names used as a per-record allowlist; `RESERVED_FIELDS` lists names the extractor never overwrites (`cmf`, `hash`, fingerprints, etc.).
+- `bingo_elastic/model/helpers.py` — file iterators (`iterate_file` generic dispatcher plus format-specific wrappers `iterate_sdf` / `iterate_smiles` / `iterate_cml`) and single-file loaders (`load_molecule`, `load_reaction`). All iterators accept `custom_properties=` (an iterable of allowed property names) and forward it to the records they yield — pass the keys of the repo's `custom_properties` mapping so extraction and the ES mapping stay aligned.
 - `tests/` — its own pytest suite with `conftest.py` fixtures that connect to `localhost:9200`.
 
 ## Core flow (the non-obvious bit)
@@ -29,6 +29,25 @@ The tautomer path is opt-in on **both sides**: `tau_search=True` on the record g
 
 Side effect to remember: fingerprints are computed at **record construction time** in the `WithIndigoObject` descriptor, not at `index_records()` time. By the time records reach the repo, the fingerprint is already frozen on the instance.
 
+## Custom SDF properties
+
+SDF `> <TAG>` lines (and any kwargs passed to `IndigoRecord(**kwargs)`) become record attributes via `WithIndigoObject` calling `iterateProperties()`. Tag capture is **opt-in**: with no `custom_properties` configured, `WithIndigoObject` skips the `iterateProperties()` loop entirely, so no SDF tags are extracted or indexed. To capture tags, pass a `custom_properties` mapping to the repo **and** the matching keys to the iterator:
+
+```python
+mapping = {"n": {"type": "integer"}, "CAS": {"type": "keyword"}}
+repo = ElasticRepository(IndexName.BINGO_MOLECULE, custom_properties=mapping)
+for rec in iterate_sdf("file.sdf", custom_properties=mapping):  # keys-only OK too
+    repo.index_record(rec)
+```
+
+The same dict drives both consumers: keys are the extraction allowlist, values are the ES `properties` fragments. `build_index_body` raises `ValueError` if a key clashes with `RESERVED_FIELDS`. Leaving `custom_properties=None` (default on both sides) means no SDF tags are extracted or indexed — only fingerprints, `cmf`, `name`, `hash`, and `has_error`. Forgetting to pass `custom_properties` to the iterator side silently drops every tag even when the repo has a typed mapping declared.
+
+Add `"index": False` to a fragment (e.g. `{"comment": {"type": "keyword", "index": False}}`) to store a value on records without making it searchable — the value stays in `_source` and is returned on retrieved records, but the field is not indexed. `build_index_body` passes the flag straight through to the ES mapping; `non_indexed_fields` (elastic.py) records which names carry it, and both repositories' `filter()` raise `ValueError` if you query one, instead of letting Elasticsearch return an opaque `search_phase_execution_exception`.
+
+`custom_properties` is validated at construction time (`validate_custom_properties`): it must be a `Dict[str, Dict]` (field name → ES fragment) or a `TypeError` is raised. A value that violates its declared ES type is not caught here — it surfaces as an `elasticsearch.helpers.BulkIndexError` at index time (`streaming_bulk` defaults to `raise_on_error=True`) rather than being silently dropped.
+
+Caveat: ES mappings are immutable after first index creation. Changing `custom_properties` later requires `ElasticRepository.delete_all_records()` first — `create_index` swallows `resource_already_exists_exception` and keeps the old mapping otherwise.
+
 ## Tests
 
 Run from `bingo/bingo-elastic/python/`:
@@ -45,7 +64,7 @@ For spinning up Elasticsearch see [claude-docs/testing.md](testing.md).
 
 ## Sync/Async parity
 
-`ElasticRepository` and `AsyncElasticRepository` are independent classes — there is no shared base. Any signature or behavior change on one must be mirrored on the other (constructors, `filter`, `index_records`, `index_record`). Tests pair every sync `test_*` with an async `test_a_*`; follow that pattern.
+`ElasticRepository` and `AsyncElasticRepository` are independent classes — there is no shared base. Any signature or behavior change on one must be mirrored on the other (constructors — including `tau_search` and `custom_properties`, `filter`, `index_records`, `index_record`). Tests pair every sync `test_*` with an async `test_a_*`; follow that pattern. Note `delete_all_records` currently exists only on the sync class; the autouse `clear_index` fixture in `tests/conftest.py` uses it to wipe both indices before every test.
 
 ## Java sibling
 

bingo/bingo-elastic/python/README.md

@@ -300,3 +300,65 @@ from bingo_elastic.queries import RangeQuery
 result = elastic_repository.filter(internal_id=RangeQuery(1000, 100000))
 
 ```
+
+#### Custom properties from SDF files
+
+SDF files can carry extra `> <TAG>` data blocks alongside each molecule. By
+default these tags are **not** read. To capture them, pass a `custom_properties`
+mapping. Its keys are the tag names to extract; its values are Elasticsearch
+[property mapping](https://www.elastic.co/guide/en/elasticsearch/reference/7.17/mapping-params.html)
+fragments, so you control how each field is stored and searched (`keyword`,
+`integer`, `float`, `text` with an `analyzer`, etc.).
+
+The same mapping must be given to **both** the repository (so the field is
+declared in the index) **and** the iterator (so the tag is extracted from the
+file). Passing it to only one side means the tag is silently dropped.
+
+```
+from bingo_elastic.model import helpers
+from bingo_elastic.elastic import ElasticRepository, IndexName
+
+custom_properties = {
+    "molecular_weight": {"type": "float"},
+    "cas_number": {"type": "keyword"},
+}
+
+repository = ElasticRepository(
+    IndexName.BINGO_MOLECULE, host="127.0.0.1", port=9200,
+    custom_properties=custom_properties,
+)
+sdf = helpers.iterate_sdf("compounds.sdf", custom_properties=custom_properties)
+repository.index_records(sdf)
+
+# Now the typed fields are searchable like any other custom field
+from bingo_elastic.queries import RangeQuery
+result = repository.filter(molecular_weight=RangeQuery(100, 500))
+```
+
+##### Storing a property without indexing it
+
+Add `"index": False` to a field's fragment to **store** the value with the
+record (it is returned when the record is retrieved) but keep it **out of the
+search index** — useful for bulky notes or metadata you never query on. Any
+attempt to `filter()` on such a field raises a `ValueError`.
+
+```
+custom_properties = {
+    "cas_number": {"type": "keyword"},                 # searchable
+    "comment": {"type": "keyword", "index": False},    # stored, not searchable
+}
+
+repository = ElasticRepository(
+    IndexName.BINGO_MOLECULE, host="127.0.0.1", port=9200,
+    custom_properties=custom_properties,
+)
+sdf = helpers.iterate_sdf("compounds.sdf", custom_properties=custom_properties)
+repository.index_records(sdf)
+
+# `comment` still comes back on retrieved records, e.g. via a similarity search,
+# but repository.filter(comment="...") raises ValueError.
+```
+
+*CAVEAT*: an index mapping is fixed once the index is first created. Changing
+`custom_properties` for an existing index has no effect until you drop it with
+`ElasticRepository.delete_all_records()` and re-index.

bingo/bingo-elastic/python/bingo_elastic/elastic.py

@@ -3,6 +3,7 @@
     Any,
     AsyncGenerator,
     Dict,
+    FrozenSet,
     Generator,
     List,
     Optional,
@@ -25,6 +26,7 @@
 from indigo import Indigo, IndigoObject  # type: ignore
 
 from bingo_elastic.model.record import (
+    RESERVED_FIELDS,
     IndigoRecord,
     IndigoRecordMolecule,
     IndigoRecordReaction,
@@ -34,6 +36,9 @@
 
 ElasticRepositoryT = TypeVar("ElasticRepositoryT")
 
+# Mapping of custom (e.g. SDF tag) field name -> ES property mapping fragment
+CustomPropertiesMapping = Dict[str, Dict[str, Any]]
+
 MAX_ALLOWED_SIZE = 1000
 
 
@@ -110,7 +115,10 @@ def get_client(
     return client_type(**arguments)  # type: ignore
 
 
-def build_index_body(tau_search: bool = False) -> Dict:
+def build_index_body(
+    tau_search: bool = False,
+    custom_properties: Optional[CustomPropertiesMapping] = None,
+) -> Dict:
     index_body = {
         "mappings": {
             "properties": {
@@ -142,9 +150,50 @@ def build_index_body(tau_search: bool = False) -> Dict:
             }
         )
 
+    if custom_properties:
+        collisions = set(custom_properties).intersection(RESERVED_FIELDS)
+        if collisions:
+            raise ValueError(
+                "custom_properties uses reserved field name(s): "
+                f"{sorted(collisions)}"
+            )
+        index_body["mappings"]["properties"].update(custom_properties)
+
     return index_body
 
 
+def non_indexed_fields(
+    custom_properties: Optional[CustomPropertiesMapping],
+) -> FrozenSet[str]:
+    """Field names mapped with "index": false.
+
+    Such fields are stored in _source (returned on retrieved records) but
+    are not searchable, so filter() must reject queries against them.
+    """
+    if not custom_properties:
+        return frozenset()
+    return frozenset(
+        name
+        for name, fragment in custom_properties.items()
+        if str(fragment.get("index", True)).lower() == "false"
+    )
+
+
+def validate_custom_properties(
+    custom_properties: Optional[CustomPropertiesMapping],
+) -> None:
+    """custom_properties must map field names to ES mapping-fragment dicts."""
+    if custom_properties is None:
+        return
+    if not isinstance(custom_properties, dict) or not all(
+        isinstance(fragment, dict) for fragment in custom_properties.values()
+    ):
+        raise TypeError(
+            "custom_properties must be a Dict[str, Dict[str, Any]] mapping "
+            "field names to Elasticsearch property fragments"
+        )
+
+
 def check_index_exception(err_: RequestError) -> None:
     if not isinstance(err_.info, dict):
         raise err_
@@ -216,7 +265,7 @@ def response_to_records(
 
 
 class AsyncElasticRepository:
-    def __init__(
+    def __init__(  # pylint: disable=too-many-arguments
         self,
         index_name: IndexName,
         *,
@@ -228,6 +277,7 @@ def __init__(
         request_timeout: int = 60,
         retry_on_timeout: bool = True,
         tau_search: bool = False,
+        custom_properties: Optional[CustomPropertiesMapping] = None,
     ) -> None:
         """
         :param index_name: use function  get_index_name for setting this argument
@@ -241,10 +291,24 @@ def __init__(
         :param tau_search: declare tau_fingerprint in the index mapping so
             tautomer-aware substructure search is available via
             filter(..., options="TAU ...")
+        :param custom_properties: ES mapping fragments for caller-defined
+            fields (SDF tags or kwargs passed to IndigoRecord). Keys are field
+            names; values are ES property mappings, e.g.
+            {"MolecularWeight": {"type": "float"}, "CAS": {"type": "keyword"}}.
+            The same keys must also be passed as ``custom_properties=`` to
+            iterate_sdf/iterate_file — without that, no SDF tags are
+            extracted and the typed mapping has nothing to populate.
+            Add ``"index": False`` to a fragment (e.g.
+            {"comment": {"type": "keyword", "index": False}}) to store the
+            value on records without making it searchable; filter() raises
+            ValueError if such a field is queried.
         """
         self.index_name = index_name.value
         self.tau_search = tau_search
-        self.index_body = build_index_body(tau_search)
+        self.custom_properties = custom_properties
+        validate_custom_properties(custom_properties)
+        self._non_indexed_fields = non_indexed_fields(custom_properties)
+        self.index_body = build_index_body(tau_search, custom_properties)
 
         self.el_client = get_client(
             client_type=AsyncElasticsearch,
@@ -276,7 +340,7 @@ async def index_records(self, records: Generator, chunk_size: int = 500):
         ):
             pass
 
-    async def filter(
+    async def filter(  # pylint: disable=too-many-locals
         self,
         query_subject: Optional[
             Union[BaseMatch, IndigoObject, IndigoRecord]
@@ -333,6 +397,13 @@ async def filter(
             raise ValueError(
                 f"page_size should less or equal to {MAX_ALLOWED_SIZE}"
             )
+        forbidden = self._non_indexed_fields.intersection(kwargs)
+        if forbidden:
+            raise ValueError(
+                f"Field(s) {sorted(forbidden)} are mapped with index=false: "
+                "stored on records but not searchable, so they cannot be "
+                "used in filter()."
+            )
         # actions needed to be called on elastic_search result
         postprocess_actions: PostprocessType = []
 
@@ -394,7 +465,7 @@ async def __aexit__(self, *args, **kwargs) -> None:
 
 
 class ElasticRepository:
-    def __init__(
+    def __init__(  # pylint: disable=too-many-arguments
         self,
         index_name: IndexName,
         *,
@@ -406,6 +477,7 @@ def __init__(
         request_timeout: int = 60,
         retry_on_timeout: bool = True,
         tau_search: bool = False,
+        custom_properties: Optional[CustomPropertiesMapping] = None,
     ) -> None:
         """
         :param index_name: use function  get_index_name for setting this argument
@@ -419,10 +491,24 @@ def __init__(
         :param tau_search: declare tau_fingerprint in the index mapping so
             tautomer-aware substructure search is available via
             filter(..., options="TAU ...")
+        :param custom_properties: ES mapping fragments for caller-defined
+            fields (SDF tags or kwargs passed to IndigoRecord). Keys are field
+            names; values are ES property mappings, e.g.
+            {"MolecularWeight": {"type": "float"}, "CAS": {"type": "keyword"}}.
+            The same keys must also be passed as ``custom_properties=`` to
+            iterate_sdf/iterate_file — without that, no SDF tags are
+            extracted and the typed mapping has nothing to populate.
+            Add ``"index": False`` to a fragment (e.g.
+            {"comment": {"type": "keyword", "index": False}}) to store the
+            value on records without making it searchable; filter() raises
+            ValueError if such a field is queried.
         """
         self.index_name = index_name.value
         self.tau_search = tau_search
-        self.index_body = build_index_body(tau_search)
+        self.custom_properties = custom_properties
+        validate_custom_properties(custom_properties)
+        self._non_indexed_fields = non_indexed_fields(custom_properties)
+        self.index_body = build_index_body(tau_search, custom_properties)
 
         self.el_client = get_client(
             client_type=Elasticsearch,
@@ -460,7 +546,7 @@ def delete_all_records(self):
         except NotFoundError:
             pass
 
-    def filter(
+    def filter(  # pylint: disable=too-many-locals
         self,
         query_subject: Optional[
             Union[BaseMatch, IndigoObject, IndigoRecord]
@@ -517,6 +603,13 @@ def filter(
             raise ValueError(
                 f"page_size should less or equal to {MAX_ALLOWED_SIZE}"
             )
+        forbidden = self._non_indexed_fields.intersection(kwargs)
+        if forbidden:
+            raise ValueError(
+                f"Field(s) {sorted(forbidden)} are mapped with index=false: "
+                "stored on records but not searchable, so they cannot be "
+                "used in filter()."
+            )
         # actions needed to be called on elastic_search result
         postprocess_actions: PostprocessType = []
         query = compile_query(