fix #50 partition by single expression raise TypeError (#52)

* fix #50 partition by single expression raise TypeError

* fix testpypi release version

Author: jayvynl

.github/workflows/base-test.yml

@@ -45,6 +45,11 @@ jobs:
         flag-name: run-${{ join(matrix.*, '-') }}
         parallel: true
 
+  release:
+
+    needs: test
+    uses: ./.github/workflows/release.yml
+
   coveralls-finish:
 
     needs: test

.github/workflows/release.yml

@@ -1,6 +1,7 @@
 name: Publish Python 🐍 distribution 📦 to PyPI and TestPyPI
 
-on: push
+on:
+  workflow_call:
 
 jobs:
   build:

.gitignore

@@ -1,5 +1,6 @@
 .coverage
 .idea
+.vscode
 .tox
 venv
 **/__pycache__

CHANGELOG.md

@@ -1,3 +1,6 @@
+### 1.1.3
+- Fix #50 partition by single expression raise TypeError.
+
 ### 1.1.2
 - Use [flake8](https://flake8.pycqa.org/) to lint code.
 - Add GitHub action which runs tests.

README.md

@@ -168,7 +168,7 @@ Notices about model definition:
 - need to specify the engine for clickhouse, specify the order_by for clickhouse order and the partition_by argument
 
 ```python
-from django.db.models import CheckConstraint, Func, IntegerChoices, Q
+from django.db.models import CheckConstraint, IntegerChoices, Q
 from django.utils import timezone
 
 from clickhouse_backend import models
@@ -180,22 +180,21 @@ class Event(models.ClickhouseModel):
         DROP = 2
         ALERT = 3
     ip = models.GenericIPAddressField(default="::")
-    ipv4 = models.GenericIPAddressField(default="127.0.0.1")
+    ipv4 = models.IPv4Field(default="127.0.0.1")
     ip_nullable = models.GenericIPAddressField(null=True)
     port = models.UInt16Field(default=0)
     protocol = models.StringField(default="", low_cardinality=True)
-    content = models.StringField(default="")
+    content = models.JSONField(default=dict)
     timestamp = models.DateTime64Field(default=timezone.now)
     created_at = models.DateTime64Field(auto_now_add=True)
     action = models.EnumField(choices=Action.choices, default=Action.PASS)
 
     class Meta:
-        verbose_name = "Network event"
-        ordering = ["-id"]
-        db_table = "event"
-        engine = models.ReplacingMergeTree(
-            order_by=["id"],
-            partition_by=Func("timestamp", function="toYYYYMMDD"),
+        ordering = ["-timestamp"]
+        engine = models.MergeTree(
+            primary_key="timestamp",
+            order_by=("timestamp", "id"),
+            partition_by=models.toYYYYMMDD("timestamp"),
             index_granularity=1024,
             index_granularity_bytes=1 << 20,
             enable_mixed_granularity_parts=1,

clickhouse_backend/__init__.py

@@ -1,3 +1,5 @@
-VERSION = (1, 1, 2)
+from clickhouse_backend.utils.version import get_version
 
-__version__ = ".".join(map(str, VERSION))
+VERSION = (1, 1, 3, "alpha", 0)
+
+__version__ = get_version(VERSION)

clickhouse_backend/backend/operations.py

@@ -5,7 +5,7 @@
 from clickhouse_backend import compat
 from clickhouse_backend.driver import JSON
 from clickhouse_backend.driver.client import insert_pattern
-from clickhouse_backend.utils import get_timezone
+from clickhouse_backend.utils.timezone import get_timezone
 
 
 class DatabaseOperations(BaseDatabaseOperations):

clickhouse_backend/models/engines.py

@@ -1,5 +1,3 @@
-from itertools import zip_longest
-
 from django.db.models import Func, Value
 from django.utils.itercompat import is_iterable
 
@@ -133,31 +131,31 @@ def __init__(
     ):
         assert (
             order_by is not None or primary_key is not None
-        ), "order_by or primary_key is missing."
+        ), "At least one of order_by or primary_key must be provided"
         self.order_by = order_by
         self.primary_key = primary_key
         self.partition_by = partition_by
 
-        if order_by is not None:
-            if isinstance(order_by, str) or not is_iterable(order_by):
-                self.order_by = [order_by]
-            if any(o is None for o in self.order_by):
-                raise ValueError("None is not allowed in order_by")
-
-        if primary_key is not None:
-            if isinstance(primary_key, str) or not is_iterable(primary_key):
-                self.primary_key = [primary_key]
-            if any(o is None for o in self.primary_key):
-                raise ValueError("None is not allowed in primary_key")
+        for key in ["order_by", "primary_key", "partition_by"]:
+            value = getattr(self, key)
+            if value is not None:
+                if isinstance(value, str) or not is_iterable(value):
+                    value = (value,)
+                    setattr(self, key, value)
+                elif not isinstance(value, tuple):
+                    value = tuple(value)
+                    setattr(self, key, value)
+                if any(i is None for i in value):
+                    raise ValueError(f"None is not allowed in {key}")
 
         # https://clickhouse.com/docs/en/engines/table-engines/mergetree-family/mergetree#choosing-a-primary-key-that-differs-from-the-sorting-key
         # primary key expression tuple must be a prefix of the sorting key expression tuple.
-        if self.order_by is not None and self.primary_key is not None:
-            for o, p in zip_longest(self.order_by, self.primary_key):
-                if p is None:
-                    break
-                if p != o:
-                    raise ValueError("primary_key must be a prefix of order_by")
+        if (
+            self.order_by is not None
+            and self.primary_key is not None
+            and self.order_by[: len(self.primary_key)] != self.primary_key
+        ):
+            raise ValueError("primary_key must be a prefix of order_by")
 
         super().__init__(*expressions, **settings)
 

clickhouse_backend/utils/__init__.py

@@ -7,10 +7,10 @@ def get_timezone():
     """Guess what timezone the user is in.
 
     Just remember, no matter USE_TZ or not,
-    clickhouse always store UNIX timestamp (which is in UTC timezone).
+    ClickHouse always store UNIX timestamp (which is in UTC timezone).
 
     Results of lookups turning datetime to date are relevant to timezone.
-    We should always pass timezone parameter to clickhouse functions like
+    We should always pass timezone parameter to ClickHouse functions like
     toDate, toStartOfMonth.
     """
     if settings.USE_TZ:

clickhouse_backend/utils.py renamed to clickhouse_backend/utils/timezone.py

@@ -0,0 +1,63 @@
+import datetime
+import functools
+import os
+import subprocess
+
+
+# Borrowed from django.utils.version
+def get_version(version):
+    """Return a PEP 440-compliant version number from VERSION."""
+
+    # Now build the two parts of the version number:
+    # main = X.Y[.Z]
+    # sub = .devN - for pre-alpha releases
+    #     | {a|b|rc}N - for alpha, beta, and rc releases
+
+    main = get_main_version(version)
+
+    sub = ""
+    if version[3] == "alpha" and version[4] == 0:
+        git_changeset = get_git_changeset()
+        if git_changeset:
+            sub = ".dev%s" % git_changeset
+
+    elif version[3] != "final":
+        mapping = {"alpha": "a", "beta": "b", "rc": "rc"}
+        sub = mapping[version[3]] + str(version[4])
+
+    return main + sub
+
+
+def get_main_version(version):
+    """Return main version (X.Y[.Z]) from VERSION."""
+    parts = 2 if version[2] == 0 else 3
+    return ".".join(str(x) for x in version[:parts])
+
+
+@functools.lru_cache(maxsize=128)
+def get_git_changeset():
+    """Return a numeric identifier of the latest git changeset.
+
+    The result is the UTC timestamp of the changeset in YYYYMMDDHHMMSS format.
+    This value isn't guaranteed to be unique, but collisions are very unlikely,
+    so it's sufficient for generating the development version numbers.
+    """
+    # Repository may not be found if __file__ is undefined, e.g. in a frozen
+    # module.
+    if "__file__" not in globals():
+        return None
+    repo_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+    git_log = subprocess.run(
+        "git log --pretty=format:%ct --quiet -1 HEAD",
+        capture_output=True,
+        shell=True,
+        cwd=repo_dir,
+        text=True,
+    )
+    timestamp = git_log.stdout
+    tz = datetime.timezone.utc
+    try:
+        timestamp = datetime.datetime.fromtimestamp(int(timestamp), tz=tz)
+    except ValueError:
+        return None
+    return timestamp.strftime("%Y%m%d%H%M%S")

clickhouse_backend/utils/version.py

@@ -15,17 +15,16 @@ class Action(IntegerChoices):
     ip_nullable = models.GenericIPAddressField(null=True)
     port = models.UInt16Field(default=0)
     protocol = models.StringField(default="", low_cardinality=True)
-    content = models.JSONField()
+    content = models.JSONField(default=dict)
     timestamp = models.DateTime64Field(default=timezone.now)
     created_at = models.DateTime64Field(auto_now_add=True)
     action = models.EnumField(choices=Action.choices, default=Action.PASS)
 
     class Meta:
-        verbose_name = "Network event"
-        ordering = ["-id"]
-        db_table = "event"
-        engine = models.ReplacingMergeTree(
-            order_by=["id"],
+        ordering = ["-timestamp"]
+        engine = models.MergeTree(
+            primary_key="timestamp",
+            order_by=("timestamp", "id"),
             partition_by=models.toYYYYMMDD("timestamp"),
             index_granularity=1024,
             index_granularity_bytes=1 << 20,
@@ -36,8 +35,8 @@ class Meta:
                 fields=["ip"], name="ip_set_idx", type=models.Set(1000), granularity=4
             ),
             models.Index(
-                fields=["port"],
-                name="port_bloom_idx",
+                fields=["ipv4"],
+                name="ipv4_bloom_idx",
                 type=models.BloomFilter(0.001),
                 granularity=1,
             ),

example/testapp/models.py

@@ -38,7 +38,7 @@ dependencies = [
 dynamic = ["version", "readme"]
 
 [tool.setuptools.dynamic]
-version = {attr = "clickhouse_backend.VERSION"}
+version = {attr = "clickhouse_backend.__version__"}
 readme = {file = ["README.md"], content-type = "text/markdown"}
 
 [tool.isort]

pyproject.toml

@@ -1,6 +1,24 @@
+from django.utils import timezone
+
 from clickhouse_backend import models
 
 
+class Event(models.ClickhouseModel):
+    ip = models.GenericIPAddressField(default="::")
+    port = models.UInt16Field(default=0)
+    protocol = models.StringField(default="", low_cardinality=True)
+    content = models.StringField(default="")
+    timestamp = models.DateTime64Field(default=timezone.now)
+
+    class Meta:
+        ordering = ["-timestamp"]
+        engine = models.MergeTree(
+            primary_key="timestamp",
+            order_by=("timestamp", "id"),
+            partition_by=models.toYYYYMMDD("timestamp"),
+        )
+
+
 class EngineWithSettings(models.ClickhouseModel):
     class Meta:
         engine = models.MergeTree(

tests/clickhouse_table_engine/models.py

@@ -1,7 +1,39 @@
 from django.db import connection
 from django.test import TestCase
 
-from .models import EngineWithSettings
+from clickhouse_backend import models
+
+from .models import EngineWithSettings, Event
+
+
+class TestMergeTree(TestCase):
+    def test_table(self):
+        opts = Event._meta
+        with connection.cursor() as cursor:
+            cursor.execute(
+                f"select engine_full from system.tables where table='{opts.db_table}'"
+            )
+            engine_full = cursor.fetchone()[0]
+        self.assertEqual(
+            engine_full.partition(" SETTINGS ")[0],
+            "MergeTree PARTITION BY toYYYYMMDD(timestamp) PRIMARY KEY timestamp ORDER BY (timestamp, id)",
+        )
+
+    def test_mergetree_init_exception(self):
+        with self.assertRaisesMessage(
+            AssertionError, "At least one of order_by or primary_key must be provided"
+        ):
+            models.MergeTree()
+        with self.assertRaisesMessage(ValueError, "None is not allowed in order_by"):
+            models.MergeTree(order_by=(None, "a"))
+        with self.assertRaisesMessage(
+            ValueError, "primary_key must be a prefix of order_by"
+        ):
+            models.MergeTree(order_by=("a", "b"), primary_key=["b"])
+        with self.assertRaisesMessage(
+            ValueError, "primary_key must be a prefix of order_by"
+        ):
+            models.MergeTree(order_by=("a", "b"), primary_key=["a", "b", "c"])
 
 
 class TestEngineSettings(TestCase):