Merge branch 'master' into fix/traversal-foundations-2.3.3

Author: dimitri-yatsenko

.github/workflows/test.yaml

@@ -68,3 +68,38 @@ jobs:
 
       - name: Run unit tests
         run: pixi run -e test pytest tests/unit -v
+
+  # Windows unit tests: guard OS-specific behavior (path separators, etc.) that
+  # the Linux jobs above cannot catch — e.g. #1520, where file-protocol paths
+  # rendered with native backslashes broke garbage collection on Windows.
+  # pixi targets linux/osx only (see [tool.pixi] platforms), so this leg uses
+  # pip. Unit tests need no containers, so no Docker/DB is required on Windows.
+  unit-tests-windows:
+    runs-on: windows-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        # Exercise both ends of the supported range (requires-python >=3.10,<3.15).
+        python-version: ["3.10", "3.14"]
+    name: unit-tests-windows (py${{ matrix.python-version }})
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 0 # hatch-vcs derives the version from git history/tags
+
+      - name: Set up Python
+        uses: actions/setup-python@v5
+        with:
+          python-version: ${{ matrix.python-version }}
+
+      # NOTE: pip resolves `[project.optional-dependencies].test`, which is a
+      # DIFFERENT set than the pixi Linux legs' `[dependency-groups].test`
+      # (e.g. it omits graphviz). A unit test importing a dep present in only
+      # one set would then pass on Linux but error on Windows (or vice versa) —
+      # a dep-set mismatch that reads like an OS bug. Keep the two `test` sets
+      # in sync when unit-test dependencies change.
+      - name: Install package with test extras
+        run: pip install -e ".[test]"
+
+      - name: Run unit tests
+        run: pytest tests/unit -v

src/datajoint/adapters/base.py

@@ -617,6 +617,22 @@ def supports_inline_indexes(self) -> bool:
         """
         return True  # Default for MySQL, override in PostgreSQL
 
+    @property
+    def auto_indexes_foreign_keys(self) -> bool:
+        """
+        Whether this backend implicitly indexes a foreign key's referencing
+        (child) columns as part of enforcing the constraint.
+
+        MySQL/InnoDB does; PostgreSQL does not (and offers no server setting to
+        make it), so DataJoint must emit an explicit index on that backend.
+
+        Returns
+        -------
+        bool
+            True for MySQL, False for PostgreSQL.
+        """
+        return True  # Default for MySQL, override in PostgreSQL
+
     def create_index_ddl(
         self,
         full_table_name: str,

src/datajoint/adapters/postgres.py

@@ -720,6 +720,14 @@ def supports_inline_indexes(self) -> bool:
         """
         return False
 
+    @property
+    def auto_indexes_foreign_keys(self) -> bool:
+        """
+        PostgreSQL never indexes a foreign key's referencing columns
+        automatically, so DataJoint emits an explicit (coverage-aware) index.
+        """
+        return False
+
     # =========================================================================
     # Introspection
     # =========================================================================

src/datajoint/declare.py

@@ -190,6 +190,7 @@ def compile_foreign_key(
     index_sql: list[str],
     adapter,
     fk_attribute_map: dict[str, tuple[str, str]] | None = None,
+    fk_index_candidates: list[list[str]] | None = None,
 ) -> None:
     """
     Parse a foreign key line and update declaration components.
@@ -215,6 +216,11 @@ def compile_foreign_key(
         Database adapter for backend-specific SQL generation.
     fk_attribute_map : dict, optional
         Mapping of ``child_attr -> (parent_table, parent_attr)``. Updated in place.
+    fk_index_candidates : list, optional
+        PostgreSQL only. Collects each non-unique foreign key's columns as a
+        candidate supporting index; the redundancy/coverage decision is made
+        post-parse (once the full primary key and index list are known).
+        Updated in place.
 
     Raises
     ------
@@ -301,10 +307,22 @@ def compile_foreign_key(
         f"FOREIGN KEY ({fk_cols}) REFERENCES {ref_table_name} ({pk_cols}) ON UPDATE CASCADE ON DELETE RESTRICT"
     )
 
-    # declare unique index
+    # Supporting index on the foreign-key columns.
+    #
+    # The `unique` option is a uniqueness constraint, always emitted, on every
+    # backend. For the non-unique case: MySQL/InnoDB indexes every foreign key
+    # implicitly, but PostgreSQL never indexes the referencing (child) columns
+    # and offers no server setting to make it (see #1512), so DataJoint must.
+    # Whether that index is *redundant* (the columns are already a left-prefix of
+    # the primary key or of another declared index) can only be decided once the
+    # whole definition is parsed, so here we merely record the candidate; the
+    # coverage decision happens post-parse in `prepare_declare`.
+    fk_attrs = list(ref.primary_key)
     if is_unique:
-        index_cols = ", ".join(adapter.quote_identifier(attr) for attr in ref.primary_key)
+        index_cols = ", ".join(adapter.quote_identifier(attr) for attr in fk_attrs)
         index_sql.append(f"UNIQUE INDEX ({index_cols})")
+    elif not adapter.auto_indexes_foreign_keys and fk_index_candidates is not None:
+        fk_index_candidates.append(fk_attrs)
 
 
 def prepare_declare(
@@ -351,6 +369,7 @@ def prepare_declare(
     external_stores = []
     fk_attribute_map = {}  # child_attr -> (parent_table, parent_attr)
     column_comments = {}  # column_name -> comment (for PostgreSQL COMMENT ON)
+    fk_index_candidates = []  # PostgreSQL: FK column-lists that may need a support index (#1512)
 
     for line in definition:
         if not line or line.startswith("#"):  # ignore additional comments
@@ -368,6 +387,7 @@ def prepare_declare(
                 index_sql,
                 adapter,
                 fk_attribute_map,
+                fk_index_candidates,
             )
         elif re.match(r"^(unique\s+)?index\s*\(.*\)\s*(#.*)?$", line, re.I):  # index
             compile_index(re.sub(r"\s*#.*$", "", line), index_sql, adapter)
@@ -383,6 +403,33 @@ def prepare_declare(
                 if comment:
                     column_comments[name] = comment
 
+    # Foreign-key support indexes (PostgreSQL; #1512). Now that the whole
+    # definition is parsed, emit an index on each candidate foreign key's columns
+    # UNLESS they are already a left-prefix of the primary key, of a declared
+    # index, or of a longer FK-support index already emitted — in which case that
+    # existing index already serves foreign-key lookups and cascades. Candidates
+    # are only collected on PostgreSQL (MySQL/InnoDB indexes FKs implicitly), so
+    # this loop is a no-op elsewhere.
+    if fk_index_candidates:
+
+        def _index_columns(index_def: str) -> list[str]:
+            match = re.match(r"(?:unique\s+)?index\s*\(([^)]+)\)", index_def, re.I)
+            return [col.strip().strip('`"') for col in match.group(1).split(",")] if match else []
+
+        # Existing prefixes an FK index could be redundant against: the primary
+        # key and every already-declared index. Grows as we accept FK indexes.
+        existing_prefixes = [list(primary_key)] + [_index_columns(s) for s in index_sql]
+
+        def _covered(candidate: list[str]) -> bool:
+            return any(prefix[: len(candidate)] == candidate for prefix in existing_prefixes)
+
+        # Longest first, so a shorter FK index left-covered by a longer one is skipped.
+        for candidate in sorted(fk_index_candidates, key=len, reverse=True):
+            if not _covered(candidate):
+                index_cols = ", ".join(adapter.quote_identifier(attr) for attr in candidate)
+                index_sql.append(f"INDEX ({index_cols})")
+                existing_prefixes.append(candidate)
+
     return (
         table_comment,
         primary_key,

src/datajoint/settings.py

@@ -466,7 +466,7 @@ def get_store_spec(self, store: str | None = None, *, use_filepath_default: bool
         # Define required and allowed keys by protocol
         required_keys: dict[str, tuple[str, ...]] = {
             "file": ("protocol", "location"),
-            "s3": ("protocol", "endpoint", "bucket", "access_key", "secret_key", "location"),
+            "s3": ("protocol", "endpoint", "bucket", "location"),
             "gcs": ("protocol", "bucket", "location"),
             "azure": ("protocol", "container", "location"),
         }

src/datajoint/storage.py

@@ -45,6 +45,27 @@ def is_url(path: str) -> bool:
     return path.lower().startswith(URL_PROTOCOLS)
 
 
+def _path_to_file_url(resolved_path: Path | PurePosixPath) -> str:
+    """
+    Convert an already-resolved absolute path to a ``file://`` URL.
+
+    Uses ``as_posix()`` so the same logic handles both POSIX paths (which
+    already start with ``/``) and Windows paths (``C:/...``, no leading
+    slash) without OS-specific branching.
+
+    Parameters
+    ----------
+    resolved_path : Path or PurePosixPath
+        Absolute, already-resolved path.
+
+    Returns
+    -------
+    str
+        ``file://`` URL.
+    """
+    return f"file:///{resolved_path.as_posix().lstrip('/')}"
+
+
 def normalize_to_url(path: str) -> str:
     """
     Normalize a path to URL form.
@@ -72,15 +93,7 @@ def normalize_to_url(path: str) -> str:
     """
     if is_url(path):
         return path
-    # Convert local path to file:// URL
-    # Ensure absolute path and proper format
-    abs_path = str(Path(path).resolve())
-    # Handle Windows paths (C:\...) vs Unix paths (/...)
-    if abs_path.startswith("/"):
-        return f"file://{abs_path}"
-    else:
-        # Windows: file:///C:/path
-        return f"file:///{abs_path.replace(chr(92), '/')}"
+    return _path_to_file_url(Path(path).resolve())
 
 
 def parse_url(url: str) -> tuple[str, str]:
@@ -327,10 +340,21 @@ def _validate_spec(self):
             if location and not Path(location).is_dir():
                 raise FileNotFoundError(f"Inaccessible local directory {location}")
         elif self.protocol == "s3":
-            required = ["endpoint", "bucket", "access_key", "secret_key"]
+            required = ["endpoint", "bucket"]
             missing = [k for k in required if not self.spec.get(k)]
             if missing:
                 raise errors.DataJointError(f"Missing S3 configuration: {', '.join(missing)}")
+            # access_key/secret_key are optional: when both are absent the
+            # underlying botocore credential chain resolves an ambient identity
+            # (instance profile, IRSA, ECS task role, SSO), matching gcs/azure.
+            # But botocore treats exactly one as a partial credential and fails
+            # late (PartialCredentialsError at first access), so reject that here
+            # with a clear message.
+            if bool(self.spec.get("access_key")) != bool(self.spec.get("secret_key")):
+                raise errors.DataJointError(
+                    "Incomplete S3 credentials: set both access_key and secret_key, "
+                    "or neither to use ambient AWS credentials."
+                )
 
     @property
     def fs(self) -> fsspec.AbstractFileSystem:
@@ -363,10 +387,14 @@ def _create_filesystem(self) -> fsspec.AbstractFileSystem:
             else:
                 endpoint_url = endpoint
 
+            # Coerce falsy (missing or empty-string) credentials to None so s3fs
+            # drops them and botocore falls through to the default chain. A
+            # forwarded "" is NOT equivalent: it survives s3fs's None-filter and
+            # botocore reads it as an explicit (invalid) credential.
             return fsspec.filesystem(
                 "s3",
-                key=self.spec["access_key"],
-                secret=self.spec["secret_key"],
+                key=self.spec.get("access_key") or None,
+                secret=self.spec.get("secret_key") or None,
                 client_kwargs={"endpoint_url": endpoint_url},
             )
 
@@ -418,7 +446,7 @@ def _full_path(self, path: str | PurePosixPath) -> str:
         elif self.protocol == "file":
             location = self.spec.get("location", "")
             if location:
-                return str(Path(location) / path)
+                return (Path(location) / path).as_posix()
             return path
         else:
             return self._require_adapter().full_path(self.spec, path)
@@ -453,13 +481,7 @@ def get_url(self, path: str | PurePosixPath) -> str:
         full_path = self._full_path(path)
 
         if self.protocol == "file":
-            # Ensure absolute path for file:// URL
-            abs_path = str(Path(full_path).resolve())
-            if abs_path.startswith("/"):
-                return f"file://{abs_path}"
-            else:
-                # Windows path
-                return f"file:///{abs_path.replace(chr(92), '/')}"
+            return _path_to_file_url(Path(full_path).resolve())
         elif self.protocol == "s3":
             return f"s3://{full_path}"
         elif self.protocol == "gcs":