Support for expressions #101

dorantor · 2018-10-17T22:16:02Z

How it's used in our project(simplified):

    $uuid = '0f372656-6a5b-4727-a4c4-f6357775d926';
    $uuidExpr = $chc->expr("UUIDStringToNum('{$uuid}')");
    $stat = $chc->insert(
        'sometable',
        ['2018-10-18', $uuidExpr],
        ['created_date', 'event_id',]
    );

Where simplified sometable is:

CREATE TABLE sometable (
    created_date Date,
    event_id FixedString(16)
) ENGINE = MergeTree(
    created_date,
    event_id,
    (created_date, event_id),
    8192
);

simPod · 2018-10-17T22:25:44Z

src/Query/Expression.php

+    /**
+     * @var string
+     */
+    protected $expression = '';


IMO there should not be a default value and not be protected. Let there only be a constructor as the way to create a valid object.

Suggested change

protected $expression = '';

private $expression;

Done. Sorry, just now understood that this change could be applied right in github.

Np, new feature introduced yesterday ;)

src/Query/Expression.php

simPod · 2018-10-17T22:26:51Z

src/Query/Expression.php

+namespace ClickHouseDB\Query;
+
+/**
+ * Query expression


This would deserve more explanation

Updated docblock with example and param type.

simPod · 2018-10-17T22:26:58Z

src/Query/Expression.php

+/**
+ * Query expression
+ *
+ * @package ClickHouseDB


This comment can be removed

src/Client.php

src/Query/Expression.php

simPod

Personally I like the idea 👍

Can we cover it with integration test so we make sure it doesn't get broken in the future? I suggest to place it into new ClickHouseDB\Tests\Query\ExpressionTest

simPod · 2018-10-17T23:31:24Z

Travis Code Style build is currently broken (will be fixed when #100 is merged) so it can be ignored

Scrutinizer however found some issues, mostly the code style https://scrutinizer-ci.com/g/smi2/phpClickHouse/inspections/e96a39a6-5929-44aa-ae63-943b77da290d/issues/

dorantor · 2018-10-18T15:38:59Z

Scrutinizer is now ok. I'll remove method expr() from client, but, as I mentioned, still think Expression creation should be encapsulated by library. But I'm not familiar enough with the library to make a good suggestion how and where it should be done.

simPod · 2018-10-18T16:06:28Z

Everything that is public = API. And even though you create another method that instantiates it, it will still be perfectly valid to instantiate it on it's own. If you want to encapsulate it, you can create something like expression builder (example https://www.doctrine-project.org/projects/doctrine-collections/en/latest/expression-builder.html).

simPod

And what do you think about the test 🤔 (#101 (review))?

src/Query/Expression.php

src/Client.php

src/Query/Expression.php

dorantor · 2018-10-18T16:15:58Z

Tests are a must. Just wanted to finalize how it will be used first. Regarding ExpressionBuilder - most probably I will not be able to do it in any close future :)

dorantor · 2018-10-18T17:36:04Z

In older version there was no call $value = ValueFormatter::formatValue($value, false); in src/Quote/StrictQuoteLine.php, so my solution was working as expected. Now it's there and converts Expression to string and applies quoting(which I try to avoid with Expression class). Currently I see two options how to make it work again:

Make Expression implementation of Type interface and return $this in getValue()
Add check in ValueFormatter::formatValue for Expression

But both options doesn't look good to me.

dorantor · 2018-10-18T17:43:54Z

Commit that introduced ValueFormatter call says

Refactor formatting

But it doesn't gives any idea why type conversion was added. W/o knowing reasons for that I cannot offer a good solution.

simPod · 2018-10-18T17:51:38Z

Yup, that cleaned up value formatting code that was spread across several classes into one

You can replicate this part for Expression and should be good 5f4fe72#diff-8b1652bb2da78b48114bdfa44b1f9b3fR35

tests/Query/ExpressionTest.php

dorantor · 2018-10-18T18:48:57Z

Yup, that cleaned up value formatting code that was spread across several classes into one

You can replicate this part for Expression and should be good 5f4fe72#diff-8b1652bb2da78b48114bdfa44b1f9b3fR35

Not sure I understood correctly. You mean make it this way?

if ($value instanceof Expression) {

simPod · 2018-10-18T18:54:55Z

Not sure I understood correctly. You mean make it this way?

Exactly. And maybe remove __toString() and replace with get(), getValue() or something similar

dorantor · 2018-10-18T19:04:44Z

Not sure I understood correctly. You mean make it this way?

Exactly. And maybe remove __toString() and replace with get(), getValue() or something similar

Main reasoning for class wrapper (Expression) is to bypass check in src/Quote/StrictQuoteLine.php:

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

If replace __toString() with getValue():string and use it inValueFormatter::formatValue() as:

        if ($value instanceof Expression) {
            return $value->getValue();
        }

check I noted before will not be bypassed.

dorantor · 2018-10-18T22:30:04Z

This is what I can offer for now, but I don't like this solution, to be honest.

simPod · 2018-10-18T23:17:18Z

What in particular you don't like about it?

simPod · 2018-10-18T23:21:55Z

src/Query/Expression.php

+class Expression
+{
+    /** @var string */
+    private $expression;


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?

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')").

kk, let's just leave it like it is

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.

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

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.

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 :)

dorantor · 2018-10-18T23:44:38Z

What in particular you don't like about it?

Cannot say. It just smells bad for me. Something is wrong here.
Validating against class instead of interface?

But it's working, so I'm not freaking out :)

simPod · 2018-10-18T23:47:02Z

I suppose some interface will emerge soon for that case. Type and Expression could have shared one.

simPod

LGTM 🚢

WDYT @isublimity?

isublimity · 2018-10-23T07:02:18Z

Merge to master
ed7ae42

dorantor added 5 commits October 10, 2017 12:16

Added Expression.

f5820dc

Merge branch 'master' of github.com:smi2/phpClickHouse

c0b5955

Merge branch 'master' into fork

b46fd48

Merge tag '1.3.1' into merge

e4bc57a

Shortened fully qualified name.

2927397

simPod suggested changes Oct 17, 2018

View reviewed changes

Updated docblocks, made internal field private, added typehint.

7336cad

simPod reviewed Oct 17, 2018

View reviewed changes

src/Query/Expression.php Outdated Show resolved Hide resolved

simPod reviewed Oct 17, 2018

View reviewed changes

simPod and others added 4 commits October 18, 2018 15:07

Update src/Query/Expression.php

57d01c5

Updated docblocks and added typehint.

9d22259

Merge branch 'merge' of github.com:dorantor/phpClickHouse into merge

41d9979

Replaced field multiline docblock with oneliner.

a46a60c

Removed expr() method from Client.

2d26997

simPod suggested changes Oct 18, 2018

View reviewed changes

src/Query/Expression.php Outdated Show resolved Hide resolved

src/Client.php Outdated Show resolved Hide resolved

src/Query/Expression.php Show resolved Hide resolved

simPod added 3 commits October 18, 2018 19:11

Update src/Query/Expression.php

45da49f

Update src/Client.php

7e25e5f

Update src/Query/Expression.php

9662a7c

dorantor added 2 commits October 18, 2018 19:16

Merge branch 'merge' of github.com:dorantor/phpClickHouse into merge

da54872

Added ExpressionTest.

c08cf6a

Fixed typo, removed empty lines.

751a501

simPod suggested changes Oct 18, 2018

View reviewed changes

simPod and others added 4 commits October 18, 2018 21:49

Update tests/Query/ExpressionTest.php

6f2259f

Update tests/Query/ExpressionTest.php

a68e3e0

Update tests/Query/ExpressionTest.php

c7944e4

Merge branch 'merge' of github.com:dorantor/phpClickHouse into merge

c4aa709

Update tests/Query/ExpressionTest.php

b3c340f

simPod and others added 4 commits October 18, 2018 22:07

Update tests/Query/ExpressionTest.php

72b7574

Update tests/Query/ExpressionTest.php

80d4196

Merge branch 'merge' of github.com:dorantor/phpClickHouse into merge

11eb84a

Fixed broken ExpressionTest.

4d8b339

Code formatting - aligned = signs as expected by phpcs.

7c14c60

simPod reviewed Oct 18, 2018

View reviewed changes

simPod approved these changes Oct 18, 2018

View reviewed changes

isublimity added the To Do label Oct 23, 2018

isublimity closed this Oct 23, 2018

dorantor mentioned this pull request Oct 25, 2018

Upgraded Expression proposal #106

Merged

Support for expressions #101

Support for expressions #101

Uh oh!

Conversation

dorantor commented Oct 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

simPod left a comment

Choose a reason for hiding this comment

Uh oh!

simPod commented Oct 17, 2018

Uh oh!

dorantor commented Oct 18, 2018

Uh oh!

simPod commented Oct 18, 2018

Uh oh!

simPod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dorantor commented Oct 18, 2018

Uh oh!

dorantor commented Oct 18, 2018

Uh oh!

dorantor commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simPod commented Oct 18, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dorantor commented Oct 18, 2018

Uh oh!

simPod commented Oct 18, 2018

Uh oh!

dorantor commented Oct 18, 2018

Uh oh!

dorantor commented Oct 18, 2018

Uh oh!

simPod commented Oct 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorantor Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorantor commented Oct 18, 2018

dorantor commented Oct 18, 2018 •

edited

Loading

dorantor Oct 18, 2018 •

edited

Loading