From 31a8aa5a61b6c41a55687ed294d8f9a67d7adfc4 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sat, 15 Jun 2024 00:52:55 +0100 Subject: [PATCH 1/8] add `Concat` function --- docs/src/piccolo/functions/string.rst | 5 +++++ piccolo/query/functions/__init__.py | 3 ++- piccolo/query/functions/string.py | 22 ++++++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/docs/src/piccolo/functions/string.rst b/docs/src/piccolo/functions/string.rst index dbc09125a..8d991c956 100644 --- a/docs/src/piccolo/functions/string.rst +++ b/docs/src/piccolo/functions/string.rst @@ -3,6 +3,11 @@ String functions .. currentmodule:: piccolo.query.functions.string +Concat +------ + +.. autoclass:: Concat + Length ------ diff --git a/piccolo/query/functions/__init__.py b/piccolo/query/functions/__init__.py index 8f8944d32..3163f6d1c 100644 --- a/piccolo/query/functions/__init__.py +++ b/piccolo/query/functions/__init__.py @@ -1,7 +1,7 @@ from .aggregate import Avg, Count, Max, Min, Sum from .datetime import Day, Extract, Hour, Month, Second, Strftime, Year from .math import Abs, Ceil, Floor, Round -from .string import Length, Lower, Ltrim, Reverse, Rtrim, Upper +from .string import Concat, Length, Lower, Ltrim, Reverse, Rtrim, Upper from .type_conversion import Cast __all__ = ( @@ -9,6 +9,7 @@ "Avg", "Cast", "Ceil", + "Concat", "Count", "Day", "Extract", diff --git a/piccolo/query/functions/string.py b/piccolo/query/functions/string.py index 556817a12..aff25a923 100644 --- a/piccolo/query/functions/string.py +++ b/piccolo/query/functions/string.py @@ -5,6 +5,11 @@ """ +import typing as t + +from piccolo.columns.base import Column +from piccolo.querystring import QueryString + from .base import Function @@ -63,6 +68,22 @@ class Upper(Function): function_name = "UPPER" +class Concat(QueryString): + def __init__( + self, + *args: t.Union[Column, QueryString, str], + alias: t.Optional[str] = None, + ): + """ + Concatenate multiple values into a single string. + """ + placeholders = ", ".join("{}" for _ in args) + + super().__init__( + template=f"CONCAT({placeholders})", *args, alias=alias + ) + + __all__ = ( "Length", "Lower", @@ -70,4 +91,5 @@ class Upper(Function): "Reverse", "Rtrim", "Upper", + "Concat", ) From 609fcb4efb89125341b6c6c8212bb2581bbd3232 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sat, 15 Jun 2024 12:07:41 +0100 Subject: [PATCH 2/8] update concat delegate --- piccolo/columns/column_types.py | 62 ++++++++++++------------------- piccolo/query/functions/string.py | 19 +++++++++- 2 files changed, 41 insertions(+), 40 deletions(-) diff --git a/piccolo/columns/column_types.py b/piccolo/columns/column_types.py index 9c74b4a52..9bf5bb6aa 100644 --- a/piccolo/columns/column_types.py +++ b/piccolo/columns/column_types.py @@ -62,7 +62,6 @@ class Band(Table): ArrayAny, ArrayNotAny, ) -from piccolo.columns.operators.string import Concat from piccolo.columns.reference import LazyTableReference from piccolo.querystring import QueryString from piccolo.utils.encoding import dump_json @@ -86,46 +85,29 @@ class ConcatDelegate: def get_querystring( self, - column_name: str, - value: t.Union[str, Varchar, Text], + column: Column, + value: t.Union[str, Column, QueryString], reverse: bool = False, ) -> QueryString: - if isinstance(value, (Varchar, Text)): - column: Column = value + """ + :param reverse: + By default the value is appended to the column's value. If + ``reverse=True`` then the value is prepended to the column's + value instead. + + """ + from piccolo.query.functions.string import Concat + + if isinstance(value, Column): if len(column._meta.call_chain) > 0: raise ValueError( "Adding values across joins isn't currently supported." ) - other_column_name = column._meta.db_column_name - if reverse: - return QueryString( - Concat.template.format( - value_1=other_column_name, value_2=column_name - ) - ) - else: - return QueryString( - Concat.template.format( - value_1=column_name, value_2=other_column_name - ) - ) - elif isinstance(value, str): - if reverse: - value_1 = QueryString("CAST({} AS text)", value) - return QueryString( - Concat.template.format(value_1="{}", value_2=column_name), - value_1, - ) - else: - value_2 = QueryString("CAST({} AS text)", value) - return QueryString( - Concat.template.format(value_1=column_name, value_2="{}"), - value_2, - ) - else: - raise ValueError( - "Only str, Varchar columns, and Text columns can be added." - ) + elif not isinstance(value, (str, QueryString)): + raise ValueError("Only str and Column values can be added.") + + args = [value, column] if reverse else [column, value] + return Concat(*args) class MathDelegate: @@ -340,12 +322,13 @@ def column_type(self): def __add__(self, value: t.Union[str, Varchar, Text]) -> QueryString: return self.concat_delegate.get_querystring( - column_name=self._meta.db_column_name, value=value + column=self, + value=value, ) def __radd__(self, value: t.Union[str, Varchar, Text]) -> QueryString: return self.concat_delegate.get_querystring( - column_name=self._meta.db_column_name, + column=self, value=value, reverse=True, ) @@ -442,12 +425,13 @@ def __init__( def __add__(self, value: t.Union[str, Varchar, Text]) -> QueryString: return self.concat_delegate.get_querystring( - column_name=self._meta.db_column_name, value=value + column=self, + value=value, ) def __radd__(self, value: t.Union[str, Varchar, Text]) -> QueryString: return self.concat_delegate.get_querystring( - column_name=self._meta.db_column_name, + column=self, value=value, reverse=True, ) diff --git a/piccolo/query/functions/string.py b/piccolo/query/functions/string.py index aff25a923..bd22a9218 100644 --- a/piccolo/query/functions/string.py +++ b/piccolo/query/functions/string.py @@ -76,11 +76,28 @@ def __init__( ): """ Concatenate multiple values into a single string. + + .. note:: + Null values are ignored, so ``null + '!!!'`` returns ``!!!``, + not ``null``. + """ + if len(args) < 2: + raise ValueError("At least two values must be passed in.") + placeholders = ", ".join("{}" for _ in args) + processed_args = [ + ( + QueryString("CAST({} AS text)", arg) + if isinstance(arg, str) + else arg + ) + for arg in args + ] + super().__init__( - template=f"CONCAT({placeholders})", *args, alias=alias + f"CONCAT({placeholders})", *processed_args, alias=alias ) From a39da37a97d8bdc597d9f58218232b09160518f6 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sat, 15 Jun 2024 12:16:09 +0100 Subject: [PATCH 3/8] go back to using the concat operator in concat delegate to keep behaviour consistent --- piccolo/columns/column_types.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/piccolo/columns/column_types.py b/piccolo/columns/column_types.py index 9bf5bb6aa..6140f6d13 100644 --- a/piccolo/columns/column_types.py +++ b/piccolo/columns/column_types.py @@ -62,6 +62,7 @@ class Band(Table): ArrayAny, ArrayNotAny, ) +from piccolo.columns.operators.string import Concat from piccolo.columns.reference import LazyTableReference from piccolo.querystring import QueryString from piccolo.utils.encoding import dump_json @@ -96,18 +97,26 @@ def get_querystring( value instead. """ - from piccolo.query.functions.string import Concat - if isinstance(value, Column): if len(column._meta.call_chain) > 0: raise ValueError( "Adding values across joins isn't currently supported." ) - elif not isinstance(value, (str, QueryString)): - raise ValueError("Only str and Column values can be added.") + elif isinstance(value, str): + value = QueryString("CAST({} AS TEXT)", value) + elif not isinstance(value, QueryString): + raise ValueError( + "Only str, Column and QueryString values can be added." + ) args = [value, column] if reverse else [column, value] - return Concat(*args) + + # We use the concat operator instead of the concat function, because + # this is what we historically used, and they treat null values + # differently. + return QueryString( + Concat.template.format(value_1="{}", value_2="{}"), *args + ) class MathDelegate: From 6b3ba49ed502692edae0906fc26bda4c0cec8c5c Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sat, 15 Jun 2024 12:21:59 +0100 Subject: [PATCH 4/8] add tests --- tests/query/functions/test_string.py | 29 ++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/tests/query/functions/test_string.py b/tests/query/functions/test_string.py index b87952634..eb84818c4 100644 --- a/tests/query/functions/test_string.py +++ b/tests/query/functions/test_string.py @@ -1,10 +1,10 @@ -from piccolo.query.functions.string import Upper +from piccolo.query.functions.string import Concat, Upper from tests.example_apps.music.tables import Band from .base import BandTest -class TestUpperFunction(BandTest): +class TestUpper(BandTest): def test_column(self): """ @@ -23,3 +23,28 @@ def test_joined_column(self): """ response = Band.select(Upper(Band.manager._.name)).run_sync() self.assertListEqual(response, [{"upper": "GUIDO"}]) + + +class TestConcat(BandTest): + + def test_column_and_string(self): + response = Band.select( + Concat(Band.name, "!!!", alias="name") + ).run_sync() + self.assertListEqual(response, [{"name": "Pythonistas!!!"}]) + + def test_column_and_column(self): + response = Band.select( + Concat(Band.name, Band.popularity, alias="name") + ).run_sync() + self.assertListEqual(response, [{"name": "Pythonistas1000"}]) + + def test_join(self): + response = Band.select( + Concat(Band.name, "-", Band.manager._.name, alias="name") + ).run_sync() + self.assertListEqual(response, [{"name": "Pythonistas-Guido"}]) + + def test_min_args(self): + with self.assertRaises(ValueError): + Concat() From 4321d0267614c02839c967f50370e50c3a09c4a4 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sat, 15 Jun 2024 20:10:40 +0100 Subject: [PATCH 5/8] fix cockroachdb test --- piccolo/query/functions/string.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/piccolo/query/functions/string.py b/piccolo/query/functions/string.py index bd22a9218..defb5fb10 100644 --- a/piccolo/query/functions/string.py +++ b/piccolo/query/functions/string.py @@ -8,6 +8,7 @@ import typing as t from piccolo.columns.base import Column +from piccolo.columns.column_types import Text, Varchar from piccolo.querystring import QueryString from .base import Function @@ -87,14 +88,16 @@ def __init__( placeholders = ", ".join("{}" for _ in args) - processed_args = [ - ( - QueryString("CAST({} AS text)", arg) - if isinstance(arg, str) - else arg - ) - for arg in args - ] + processed_args = [] + + for arg in args: + if isinstance(arg, str) or ( + isinstance(arg, Column) + and not isinstance(arg, (Varchar, Text)) + ): + processed_args.append(QueryString("CAST({} AS TEXT)", arg)) + else: + processed_args.append(arg) super().__init__( f"CONCAT({placeholders})", *processed_args, alias=alias From 7e3803c58550e9c4c07cf720a2a36a91c073a7e1 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sat, 15 Jun 2024 20:16:51 +0100 Subject: [PATCH 6/8] fix mypy error --- piccolo/query/functions/string.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/piccolo/query/functions/string.py b/piccolo/query/functions/string.py index defb5fb10..30dd61d67 100644 --- a/piccolo/query/functions/string.py +++ b/piccolo/query/functions/string.py @@ -88,7 +88,7 @@ def __init__( placeholders = ", ".join("{}" for _ in args) - processed_args = [] + processed_args: t.List[t.Union[QueryString, Column]] = [] for arg in args: if isinstance(arg, str) or ( From e58f882be96ed4b08bb6900379fd4573fd87622b Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sat, 15 Jun 2024 20:39:31 +0100 Subject: [PATCH 7/8] add notes that concat isn't available in older sqlite versions --- piccolo/query/functions/string.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/piccolo/query/functions/string.py b/piccolo/query/functions/string.py index 30dd61d67..a881f57a7 100644 --- a/piccolo/query/functions/string.py +++ b/piccolo/query/functions/string.py @@ -7,9 +7,12 @@ import typing as t +import pytest + from piccolo.columns.base import Column from piccolo.columns.column_types import Text, Varchar from piccolo.querystring import QueryString +from tests.base import engine_version_lt, is_running_sqlite from .base import Function @@ -69,6 +72,10 @@ class Upper(Function): function_name = "UPPER" +@pytest.mark.skipif( + is_running_sqlite() and engine_version_lt(3.44), + reason="SQLite version not supported", +) class Concat(QueryString): def __init__( self, @@ -82,6 +89,9 @@ def __init__( Null values are ignored, so ``null + '!!!'`` returns ``!!!``, not ``null``. + .. warning:: + For SQLite, this is only available in version 3.44.0 and above. + """ if len(args) < 2: raise ValueError("At least two values must be passed in.") From 3f483bec7d75794bec5f600368a158bd1f52fba5 Mon Sep 17 00:00:00 2001 From: Daniel Townsend Date: Sat, 15 Jun 2024 20:42:47 +0100 Subject: [PATCH 8/8] move decorator to right file --- piccolo/query/functions/string.py | 7 ------- tests/query/functions/test_string.py | 7 +++++++ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/piccolo/query/functions/string.py b/piccolo/query/functions/string.py index a881f57a7..68b78219f 100644 --- a/piccolo/query/functions/string.py +++ b/piccolo/query/functions/string.py @@ -7,12 +7,9 @@ import typing as t -import pytest - from piccolo.columns.base import Column from piccolo.columns.column_types import Text, Varchar from piccolo.querystring import QueryString -from tests.base import engine_version_lt, is_running_sqlite from .base import Function @@ -72,10 +69,6 @@ class Upper(Function): function_name = "UPPER" -@pytest.mark.skipif( - is_running_sqlite() and engine_version_lt(3.44), - reason="SQLite version not supported", -) class Concat(QueryString): def __init__( self, diff --git a/tests/query/functions/test_string.py b/tests/query/functions/test_string.py index eb84818c4..bd3a8c2ab 100644 --- a/tests/query/functions/test_string.py +++ b/tests/query/functions/test_string.py @@ -1,4 +1,7 @@ +import pytest + from piccolo.query.functions.string import Concat, Upper +from tests.base import engine_version_lt, is_running_sqlite from tests.example_apps.music.tables import Band from .base import BandTest @@ -25,6 +28,10 @@ def test_joined_column(self): self.assertListEqual(response, [{"upper": "GUIDO"}]) +@pytest.mark.skipif( + is_running_sqlite() and engine_version_lt(3.44), + reason="SQLite version not supported", +) class TestConcat(BandTest): def test_column_and_string(self):