Merge branch 'master' into issue151

dimitri-yatsenko · dimitri-yatsenko · commit 33be38437685 · 2022-01-19T12:11:20.000-06:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,11 +1,18 @@
 ## Release notes
 
 ### 0.13.3 -- TBD
+* Bugfix - Fix Python 3.10 compatibility (#983) PR #972
+* Bugfix - Allow renaming non-conforming attributes in proj (#982) PR #972
+* Add - Expose proxy feature for S3 external stores (#961) PR #962
 * Bugfix - Dependencies not properly loaded on populate. (#902) PR #919
 * Bugfix - Replace use of numpy aliases of built-in types with built-in type. (#938) PR #939
 * Bugfix - Deletes and drops must include the master of each part. (#151 and #374) PR #957
 * Bugfix - `ExternalTable.delete` should not remove row on error (#953) PR #956
 * Bugfix - Fix error handling of remove_object function in `s3.py` (#952) PR #955
+* Bugfix - Fix regression issue with `DISTINCT` clause and `GROUP_BY` (#914) PR #963
+* Bugfix - Fix sql code generation to comply with sql mode `ONLY_FULL_GROUP_BY` (#916) PR #965
+* Bugfix - Fix count for left-joined `QueryExpressions` (#951) PR #966
+* Bugfix - Fix assertion error when performing a union into a join (#930) PR #967
 
 ### 0.13.2 -- May 7, 2021
 * Update `setuptools_certificate` dependency to new name `otumat`
diff --git a/LNX-docker-compose.yml b/LNX-docker-compose.yml
@@ -32,7 +32,7 @@ services:
       interval: 1s
   fakeservices.datajoint.io:
     <<: *net
-    image: datajoint/nginx:v0.0.18
+    image: datajoint/nginx:v0.0.19
     environment:
     - ADD_db_TYPE=DATABASE
     - ADD_db_ENDPOINT=db:3306
diff --git a/datajoint/declare.py b/datajoint/declare.py
@@ -11,7 +11,7 @@
 
 UUID_DATA_TYPE = 'binary(16)'
 MAX_TABLE_NAME_LENGTH = 64
-CONSTANT_LITERALS = {'CURRENT_TIMESTAMP'}  # SQL literals to be used without quotes (case insensitive)
+CONSTANT_LITERALS = {'CURRENT_TIMESTAMP', 'NULL'}  # SQL literals to be used without quotes (case insensitive)
 EXTERNAL_TABLE_ROOT = '~external'
 
 TYPE_PATTERN = {k: re.compile(v, re.I) for k, v in dict(
diff --git a/datajoint/diagram.py b/datajoint/diagram.py
@@ -296,7 +296,7 @@ def make_dot(self):
                 node.set_style('filled')
 
             for edge in dot.get_edges():
-                # see http://www.graphviz.org/content/attrs
+                # see https://graphviz.org/doc/info/attrs.html
                 src = edge.get_source().strip('"')
                 dest = edge.get_destination().strip('"')
                 props = graph.get_edge_data(src, dest)
diff --git a/datajoint/expression.py b/datajoint/expression.py
@@ -9,6 +9,7 @@
 from .preview import preview, repr_html
 from .condition import AndList, Not, \
     make_condition, assert_join_compatibility, extract_column_names, PromiscuousOperand
+from .declare import CONSTANT_LITERALS
 
 logger = logging.getLogger(__name__)
 
@@ -44,6 +45,9 @@ class QueryExpression:
     _heading = None
     _support = None
 
+    # If the query will be using distinct
+    _distinct = False
+
     @property
     def connection(self):
         """ a dj.Connection object """
@@ -106,9 +110,8 @@ def make_sql(self, fields=None):
         Make the SQL SELECT statement.
         :param fields: used to explicitly set the select attributes
         """
-        distinct = self.heading.names == self.primary_key
         return 'SELECT {distinct}{fields} FROM {from_}{where}'.format(
-            distinct="DISTINCT " if distinct else "",
+            distinct="DISTINCT " if self._distinct else "",
             fields=self.heading.as_sql(fields or self.heading.names),
             from_=self.from_clause(), where=self.where_clause())
 
@@ -266,9 +269,11 @@ def join(self, other, semantic_check=True, left=False):
             - join_attributes)
         # need subquery if any of the join attributes are derived
         need_subquery1 = (need_subquery1 or isinstance(self, Aggregation) or
-                          any(n in self.heading.new_attributes for n in join_attributes))
+                          any(n in self.heading.new_attributes for n in join_attributes)
+                          or isinstance(self, Union))
         need_subquery2 = (need_subquery2 or isinstance(other, Aggregation) or
-                          any(n in other.heading.new_attributes for n in join_attributes))
+                          any(n in other.heading.new_attributes for n in join_attributes)
+                          or isinstance(self, Union))
         if need_subquery1:
             self = self.make_subquery()
         if need_subquery2:
@@ -309,9 +314,9 @@ def proj(self, *attributes, **named_attributes):
         Each attribute name can only be used once.
         """
         # new attributes in parentheses are included again with the new name without removing original
-        duplication_pattern = re.compile(r'\s*\(\s*(?P<name>[a-z][a-z_0-9]*)\s*\)\s*$')
+        duplication_pattern = re.compile(fr'^\s*\(\s*(?!{"|".join(CONSTANT_LITERALS)})(?P<name>[a-zA-Z_]\w*)\s*\)\s*$')
         # attributes without parentheses renamed
-        rename_pattern = re.compile(r'\s*(?P<name>[a-z][a-z_0-9]*)\s*$')
+        rename_pattern = re.compile(fr'^\s*(?!{"|".join(CONSTANT_LITERALS)})(?P<name>[a-zA-Z_]\w*)\s*$')
         replicate_map = {k: m.group('name')
                          for k, m in ((k, duplication_pattern.match(v)) for k, v in named_attributes.items()) if m}
         rename_map = {k: m.group('name')
@@ -440,8 +445,10 @@ def tail(self, limit=25, **fetch_kwargs):
     def __len__(self):
         """:return: number of elements in the result set e.g. ``len(q1)``."""
         return self.connection.query(
-            'SELECT count(DISTINCT {fields}) FROM {from_}{where}'.format(
-                fields=self.heading.as_sql(self.primary_key, include_aliases=False),
+            'SELECT {select_} FROM {from_}{where}'.format(
+                select_=('count(*)' if any(self._left)
+                         else 'count(DISTINCT {fields})'.format(fields=self.heading.as_sql(
+                            self.primary_key, include_aliases=False))),
                 from_=self.from_clause(),
                 where=self.where_clause())).fetchone()[0]
 
@@ -554,7 +561,7 @@ def create(cls, arg, group, keep_all_rows=False):
         if inspect.isclass(group) and issubclass(group, QueryExpression):
             group = group()   # instantiate if a class
         assert isinstance(group, QueryExpression)
-        if keep_all_rows and len(group.support) > 1:
+        if keep_all_rows and len(group.support) > 1 or group.heading.new_attributes:
             group = group.make_subquery()  # subquery if left joining a join
         join = arg.join(group, left=keep_all_rows)  # reuse the join logic
         result = cls()
@@ -718,6 +725,7 @@ def __and__(self, other):
         if not isinstance(other, QueryExpression):
             raise DataJointError('Set U can only be restricted with a QueryExpression.')
         result = copy.copy(other)
+        result._distinct = True
         result._heading = result.heading.set_primary_key(self.primary_key)
         result = result.proj()
         return result
diff --git a/datajoint/external.py b/datajoint/external.py
@@ -1,5 +1,5 @@
 from pathlib import Path, PurePosixPath, PureWindowsPath
-from collections import Mapping
+from collections.abc import Mapping
 from tqdm import tqdm
 from .settings import config
 from .errors import DataJointError, MissingExternalFile
diff --git a/datajoint/s3.py b/datajoint/s3.py
@@ -3,6 +3,7 @@
 """
 from io import BytesIO
 import minio   # https://docs.minio.io/docs/python-client-api-reference
+import urllib3
 import warnings
 import uuid
 import logging
@@ -16,9 +17,24 @@ class Folder:
     """
     A Folder instance manipulates a flat folder of objects within an S3-compatible object store
     """
-    def __init__(self, endpoint, bucket, access_key, secret_key, *, secure=False, **_):
-        self.client = minio.Minio(endpoint, access_key=access_key, secret_key=secret_key,
-                                  secure=secure)
+    def __init__(self, endpoint, bucket, access_key, secret_key, *, secure=False,
+                 proxy_server=None, **_):
+        # from https://docs.min.io/docs/python-client-api-reference
+        self.client = minio.Minio(
+            endpoint,
+            access_key=access_key,
+            secret_key=secret_key,
+            secure=secure,
+            http_client=(
+                urllib3.ProxyManager(proxy_server,
+                                     timeout=urllib3.Timeout.DEFAULT_TIMEOUT,
+                                     cert_reqs="CERT_REQUIRED",
+                                     retries=urllib3.Retry(total=5,
+                                                           backoff_factor=0.2,
+                                                           status_forcelist=[500, 502, 503,
+                                                                             504]))
+                if proxy_server else None),
+            )
         self.bucket = bucket
         if not self.client.bucket_exists(bucket):
             raise errors.BucketInaccessible('Inaccessible s3 bucket %s' % bucket)
diff --git a/datajoint/settings.py b/datajoint/settings.py
@@ -58,7 +58,7 @@
 }
 
 
-class Config(collections.MutableMapping):
+class Config(collections.abc.MutableMapping):
 
     instance = None
 
@@ -137,7 +137,8 @@ def get_store_spec(self, store):
         spec['subfolding'] = spec.get('subfolding', DEFAULT_SUBFOLDING)
         spec_keys = {  # REQUIRED in uppercase and allowed in lowercase
             'file': ('PROTOCOL', 'LOCATION', 'subfolding', 'stage'),
-            's3': ('PROTOCOL', 'ENDPOINT', 'BUCKET', 'ACCESS_KEY', 'SECRET_KEY', 'LOCATION', 'secure', 'subfolding', 'stage')}
+            's3': ('PROTOCOL', 'ENDPOINT', 'BUCKET', 'ACCESS_KEY', 'SECRET_KEY', 'LOCATION',
+                   'secure', 'subfolding', 'stage', 'proxy_server')}
 
         try:
             spec_keys = spec_keys[spec.get('protocol', '').lower()]
diff --git a/docs-parts/intro/Releases_lang1.rst b/docs-parts/intro/Releases_lang1.rst
@@ -1,10 +1,17 @@
 0.13.3 -- TBD
 ----------------------
+* Bugfix - Fix Python 3.10 compatibility (#983) PR #972
+* Bugfix - Allow renaming non-conforming attributes in proj (#982) PR #972
+* Add - Expose proxy feature for S3 external stores (#961) PR #962
 * Bugfix - Dependencies not properly loaded on populate. (#902) PR #919
 * Bugfix - Replace use of numpy aliases of built-in types with built-in type. (#938) PR #939
 * Bugfix - Deletes and drops must include the master of each part. (#151 and #374) PR #957
 * Bugfix - `ExternalTable.delete` should not remove row on error (#953) PR #956
 * Bugfix - Fix error handling of remove_object function in `s3.py` (#952) PR #955
+* Bugfix - Fix sql code generation to comply with sql mode ``ONLY_FULL_GROUP_BY`` (#916) PR #965
+* Bugfix - Fix count for left-joined ``QueryExpressions`` (#951) PR #966
+* Bugfix - Fix assertion error when performing a union into a join (#930) PR #967
+* Bugfix - Fix regression issue with `DISTINCT` clause and `GROUP_BY` (#914) PR #963
 
 0.13.2 -- May 7, 2021
 ----------------------
diff --git a/local-docker-compose.yml b/local-docker-compose.yml
@@ -34,7 +34,7 @@ services:
       interval: 1s
   fakeservices.datajoint.io:
     <<: *net
-    image: datajoint/nginx:v0.0.18
+    image: datajoint/nginx:v0.0.19
     environment:
     - ADD_db_TYPE=DATABASE
     - ADD_db_ENDPOINT=db:3306
diff --git a/requirements.txt b/requirements.txt
@@ -10,3 +10,4 @@ minio>=7.0.0
 matplotlib
 cryptography
 otumat
+urllib3
diff --git a/tests/schema.py b/tests/schema.py
@@ -439,3 +439,13 @@ class SessionDateA(dj.Lookup):
         ('mouse1', '2020-12-03'),
         ('mouse1', '2020-12-04')
     ]
+
+
+@schema
+class Stimulus(dj.Lookup):
+    definition = """
+    id: int
+    ---
+    contrast: int
+    brightness: int
+    """
diff --git a/tests/test_aggr_regressions.py b/tests/test_aggr_regressions.py
@@ -3,10 +3,11 @@
 """
 
 import itertools
-from nose.tools import assert_equal, raises
+from nose.tools import assert_equal
 import datajoint as dj
 from . import PREFIX, CONN_INFO
-
+import uuid
+from .schema_uuid import Topic, Item, top_level_namespace_id
 schema = dj.Schema(PREFIX + '_aggr_regress', connection=dj.conn(**CONN_INFO))
 
 # --------------- ISSUE 386 -------------------
@@ -103,3 +104,27 @@ def test_issue558_part2():
     d = dict(id=3, id2=5)
     assert_equal(len(X & d), len((X & d).proj(id2='3')))
 
+
+def test_left_join_len():
+    Topic().add('jeff')
+    Item.populate()
+    Topic().add('jeff2')
+    Topic().add('jeff3')
+    q = Topic.join(Item - dict(topic_id=uuid.uuid5(top_level_namespace_id, 'jeff')),
+                   left=True)
+    qf = q.fetch()
+    assert len(q) == len(qf)
+
+
+def test_union_join():
+    # https://github.com/datajoint/datajoint-python/issues/930
+    A.insert(zip([100, 200, 300, 400, 500, 600]))
+    B.insert([(100, 11), (200, 22), (300, 33), (400, 44)])
+    q1 = B & 'id < 300'
+    q2 = B & 'id > 300'
+
+    expected_data = [{'id': 0, 'id2': 5}, {'id': 1, 'id2': 6}, {'id': 2, 'id2': 7},
+                     {'id': 3, 'id2': 8}, {'id': 4, 'id2': 9}, {'id': 100, 'id2': 11},
+                     {'id': 200, 'id2': 22}, {'id': 400, 'id2': 44}]
+
+    assert ((q1 + q2) * A).fetch(as_dict=True) == expected_data
diff --git a/tests/test_fetch.py b/tests/test_fetch.py
@@ -6,6 +6,7 @@
 import pandas
 import warnings
 from . import schema
+from .schema import Parent, Stimulus
 import datajoint as dj
 import os
 
@@ -287,3 +288,34 @@ def test_query_caching(self):
 
         # reset cache directory state (will fail if purge was unsuccessful)
         os.rmdir(os.path.expanduser('~/dj_query_cache'))
+
+    def test_fetch_group_by(self):
+        # https://github.com/datajoint/datajoint-python/issues/914
+
+        assert Parent().fetch('KEY', order_by='name') == [{'parent_id': 1}]
+
+    def test_dj_u_distinct(self):
+        # Test developed to see if removing DISTINCT from the select statement
+        # generation breakes the dj.U universal set imlementation
+
+        # Contents to be inserted
+        contents = [
+            (1,2,3),
+            (2,2,3),
+            (3,3,2),
+            (4,5,5)
+        ]
+        Stimulus.insert(contents)
+
+        # Query the whole table
+        test_query = Stimulus()
+
+        # Use dj.U to create a list of unique contrast and brightness combinations
+        result = dj.U('contrast', 'brightness') & test_query
+        expected_result = [{'contrast': 2, 'brightness': 3},
+                           {'contrast': 3, 'brightness': 2},
+                           {'contrast': 5, 'brightness': 5}]
+
+        fetched_result = result.fetch(as_dict=True, order_by=('contrast', 'brightness'))
+        Stimulus.delete_quick()
+        assert fetched_result == expected_result
diff --git a/tests/test_groupby.py b/tests/test_groupby.py
@@ -0,0 +1,11 @@
+from .schema_simple import A, D
+
+
+def test_aggr_with_proj():
+    # issue #944 - only breaks with MariaDB
+    # MariaDB implements the SQL:1992 standard that prohibits fields in the select statement that are 
+    # not also in the GROUP BY statement.
+    # An improved specification in SQL:2003 allows fields that are functionally dependent on the group by 
+    # attributes to be allowed in the select. This behavior is implemented by MySQL correctly but not MariaDB yet. 
+    # See MariaDB issue: https://jira.mariadb.org/browse/MDEV-11588
+    A.aggr(D.proj(m='id_l'), ..., n='max(m) - min(m)')
diff --git a/tests/test_relational_operand.py b/tests/test_relational_operand.py
@@ -13,6 +13,8 @@
 from .schema import (Experiment, TTest3, Trial, Ephys, Child, Parent, SubjectA, SessionA,
                      SessionStatusA, SessionDateA)
 
+from . import PREFIX, CONN_INFO
+
 
 def setup():
     """
@@ -177,6 +179,15 @@ def test_project():
         assert_equal(len((D() & cond).proj()), len((D() & cond)),
                      'projection failed: altered its argument''s cardinality')
 
+    @staticmethod
+    def test_rename_non_dj_attribute():
+        schema = PREFIX + '_test1'
+        connection = dj.conn(**CONN_INFO)
+        connection.query(f'CREATE TABLE {schema}.test_table (oldID int PRIMARY KEY)').fetchall()
+        mySchema = dj.VirtualModule(schema, schema)
+        assert 'oldID' not in  mySchema.TestTable.proj(new_name='oldID').heading.attributes.keys(), 'Failed to rename attribute correctly'
+        connection.query(f'DROP TABLE {schema}.test_table')
+
     @staticmethod
     def test_union():
         x = set(zip(*IJ.fetch('i', 'j')))
diff --git a/tests/test_s3.py b/tests/test_s3.py
@@ -70,7 +70,7 @@ def test_remove_object_exception():
 
         # Apply our new minio client which has a user that does not exist
         schema.external['share'].s3.client = Minio(
-            'minio:9000',
+            S3_CONN_INFO['endpoint'],
             access_key='jeffjeff',
             secret_key='jeffjeff',
             secure=False)
