Skip to content

Support for expressions #101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 27 commits into from
Closed

Support for expressions #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
f5820dc
Added Expression.
dorantor Oct 10, 2017
c0b5955
Merge branch 'master' of github.com:smi2/phpClickHouse
dorantor Mar 13, 2018
b46fd48
Merge branch 'master' into fork
dorantor Mar 13, 2018
e4bc57a
Merge tag '1.3.1' into merge
dorantor Oct 17, 2018
2927397
Shortened fully qualified name.
dorantor Oct 17, 2018
7336cad
Updated docblocks, made internal field private, added typehint.
dorantor Oct 17, 2018
57d01c5
Update src/Query/Expression.php
simPod Oct 18, 2018
9d22259
Updated docblocks and added typehint.
dorantor Oct 18, 2018
41d9979
Merge branch 'merge' of github.com:dorantor/phpClickHouse into merge
dorantor Oct 18, 2018
a46a60c
Replaced field multiline docblock with oneliner.
dorantor Oct 18, 2018
2d26997
Removed expr() method from Client.
dorantor Oct 18, 2018
45da49f
Update src/Query/Expression.php
simPod Oct 18, 2018
7e25e5f
Update src/Client.php
simPod Oct 18, 2018
9662a7c
Update src/Query/Expression.php
simPod Oct 18, 2018
da54872
Merge branch 'merge' of github.com:dorantor/phpClickHouse into merge
dorantor Oct 18, 2018
c08cf6a
Added ExpressionTest.
dorantor Oct 18, 2018
751a501
Fixed typo, removed empty lines.
dorantor Oct 18, 2018
6f2259f
Update tests/Query/ExpressionTest.php
simPod Oct 18, 2018
a68e3e0
Update tests/Query/ExpressionTest.php
simPod Oct 18, 2018
c7944e4
Update tests/Query/ExpressionTest.php
simPod Oct 18, 2018
c4aa709
Merge branch 'merge' of github.com:dorantor/phpClickHouse into merge
dorantor Oct 18, 2018
b3c340f
Update tests/Query/ExpressionTest.php
simPod Oct 18, 2018
72b7574
Update tests/Query/ExpressionTest.php
simPod Oct 18, 2018
80d4196
Update tests/Query/ExpressionTest.php
simPod Oct 18, 2018
11eb84a
Merge branch 'merge' of github.com:dorantor/phpClickHouse into merge
dorantor Oct 18, 2018
4d8b339
Fixed broken ExpressionTest.
dorantor Oct 18, 2018
7c14c60
Code formatting - aligned = signs as expected by phpcs.
dorantor Oct 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
include_once __DIR__ . '/src/Query/WriteToFile.php';
include_once __DIR__ . '/src/Query/WhereInFile.php';
include_once __DIR__ . '/src/Query/Query.php';
include_once __DIR__ . '/src/Query/Expression.php';
// Transport
include_once __DIR__ . '/src/Transport/Http.php';
include_once __DIR__ . '/src/Transport/CurlerRolling.php';
Expand Down
25 changes: 25 additions & 0 deletions src/Query/Expression.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

declare(strict_types=1);

namespace ClickHouseDB\Query;

/**
* Pass expression "as is" to be sent and executed at server.
* P.ex.: `new Expression("UUIDStringToNum('0f372656-6a5b-4727-a4c4-f6357775d926')");`
*/
class Expression
{
/** @var string */
private $expression;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing is on my mind. This Expression you introduced could easily replace my Type I introduced here #94.

If $expression would be of type mixed and we dropped __toString() and replaced with something like getValue() (@return mixed) I could then drop those classes of mine in next PR.

My intention was to use it like this

UInt64::fromString('18446744073709551615')

but now it could be replaced with

new Expression('18446744073709551615')

for lower numbers eg. new Expression(1844674)

My main point is, why to limit the expression only for strings?

Copy link
Contributor Author

@dorantor dorantor Oct 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was different intention and reasoning for Type and Expression.
Expression was made to bypass quoting. Inspired by other DB-related libs. But, btw, not necessarily in a best way to implement this intention. I just found it less obtrusive and easier to merge back to my fork in future.
Type, as I can see, introduced for different reason - to allow safely and in a controlled fashion add new, potentially complex types(what about Date type, f.ex.?).

While Expression seems to be universal here I'm against replacing Type with Expression, because Expression is a last resort. Unsafe one, I must say. Should be used only when there are no other options. In theory, with time, Expression could be replaced with some support for functions from library itself. Like
new Function\UUIDStringToNum($uuid)
instead of
new Expression("UUIDStringToNum('0f372656-6a5b-4727-a4c4-f6357775d926')").

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kk, let's just leave it like it is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, maybe, Expression could become either a higher level (abstract?) class or even Interface and Type can be a descendant to it. Because you're right in one aspect - Expression could solve a wider range of requirements including ones that are solved by Type.
But it will require to refactor this part in src/Quote/StrictQuoteLine.php:

            $value = ValueFormatter::formatValue($value, false);

            if (is_float($value) || is_int($value)) {
                return (string) $value;
            }

            if (is_string($value) && $encode) {

if you want Expression::getValue() to return string. Because otherwise string
"UUIDStringToNum('0f372656-6a5b-4727-a4c4-f6357775d926')"
will become
'\'UUIDStringToNum(\\\'0f372656-6a5b-4727-a4c4-f6357775d926\\\')\''
after quoting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right it might need some tweak in that part of code. But see this line

https://github.com/smi2/phpClickHouse/blob/master/src/Quote/StrictQuoteLine.php#L74

If replaced by if ($value instanceof Expression) { it might do the trick.

BTW I'd definitely be for interface Expression rather than abstract class Expression

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, missed that one. So it should be something like this IMO:

            if ($value instanceof NumericType) {
                $encode = $value->doQuoting();
            }

But in this case abstract class AND interface required. Abstract is required to define default doQuoting() : boolean. I think I can do it after this one is merged. Because, obviously, it will be refactoring of Type and Expression.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I'd name it something like needsEncoding() : bool and abstract class is IMO not required, we don't really need default value so interface will do. But for sure in next PR :)


public function __construct(string $expression)
{
$this->expression = $expression;
}

public function __toString() : string
{
return $this->expression;
}
}
5 changes: 5 additions & 0 deletions src/Quote/ValueFormatter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
namespace ClickHouseDB\Quote;

use ClickHouseDB\Exception\UnsupportedValueType;
use ClickHouseDB\Query\Expression;
use ClickHouseDB\Type\Type;
use DateTimeInterface;
use function addslashes;
Expand Down Expand Up @@ -36,6 +37,10 @@ public static function formatValue($value, bool $addQuotes = true)
return $value->getValue();
}

if ($value instanceof Expression) {
return $value;
}

if (is_object($value) && is_callable([$value, '__toString'])) {
$value = (string) $value;
}
Expand Down
34 changes: 34 additions & 0 deletions tests/Query/ExpressionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

declare(strict_types=1);

namespace ClickHouseDB\Tests\Query;

use ClickHouseDB\Query\Expression;
use ClickHouseDB\Quote\FormatLine;
use PHPUnit\Framework\TestCase;

final class ExpressionTest extends TestCase
{
public function testToString() : void
{
$expressionString = "UUIDStringToNum('0f372656-6a5b-4727-a4c4-f6357775d926')";
$expressionObject = new Expression($expressionString);

self::assertEquals(
$expressionString,
(string) $expressionObject
);
}

public function testExpressionValueForInsert() : void
{
$expressionString = "UUIDStringToNum('0f372656-6a5b-4727-a4c4-f6357775d926')";
$preparedValue = FormatLine::Insert([new Expression($expressionString)]);

self::assertEquals(
$expressionString,
$preparedValue
);
}
}