Allow internal functions to be overridden. #6500

ThePixelDeveloper · 2017-06-13T09:19:43Z

Background

This came about because we're writing a driver for PrestoDB and wanted to implement windowing for the aggregate functions. This is fine for anything new, but we struggled when trying to add windowing support for count, avg, etc ...

All Aggregate Functions can be used as window functions by adding the OVER clause. The aggregate function is computed for each row over the rows within the current row’s window frame.

Source: 6.15. Window Functions — Presto 0.178 Documentation

Solution

Obvious solution for us was to have the ability to override the internal aggregate functions. We moved the internal aggregate functions into the $_NUMERIC_FUNCTIONS property so they can be overridden; They were handled as a special case before.

The only part we weren't sure of was the dctrn alias used: https://github.com/SamKnows/doctrine2/pull/1/files#diff-0c8825d1265a66bb8b20f3d99c276961

If that doesn't matter and we can go with sclr, then this is another path that can be removed.

Thoughts?

PS. First big PR for the Doctrine internals 👍

Ocramius · 2017-06-13T11:53:17Z

Overall, this removes the internal and non-internal functions separation, which is quite an improvement.

I'm not sure if I'd like folks to override stuff like MAX or AVG though: lots of space for hurting yourself there... Also, the SqlWalker still treats them as "special" via instanceof checks, so it's not like they became reusable or replaceable anyway...

ThePixelDeveloper · 2017-06-14T13:19:16Z

Also, the SqlWalker still treats them as "special" via instanceof checks, so it's not like they became reusable or replaceable anyway...

The only reason there's a case in SqlWalker is because of the dctrn alias. I didn't know if it had special meaning within Doctrine. If it doesn't, I'll remove that case statement and let it be handled by the one below it.

I'm not sure if I'd like folks to override stuff like MAX or AVG though: lots of space for hurting yourself there...

I'm not sure I agree, there's plenty of other places to shoot yourself in the foot already. Custom SQL Walkers allow us to rewrite the output SQL in ways that don't relate to the SQL we write.

I'd rather have a clean / consistent way to override functionality. Before I came up with this solution I was trying to think of alternatives via custom SQL Walkers, which would have been far more magical. At least with this I can look for a DQL function with the key count and see what the new logic is.

Ocramius · 2017-06-19T00:36:00Z

Just a note here: patch looks very much clean and IMO it should be merged, but I'd like to have @beberlei, @guilhermeblanco and @lcobucci look at it first.

guilhermeblanco · 2017-06-19T01:07:19Z

LGTM.

Ocramius · 2017-06-19T01:37:16Z

lib/Doctrine/ORM/ORMException.php

-     *
-     * @return ORMException
-     */
-    public static function overwriteInternalDQLFunctionNotAllowed($functionName)


This method removal is effectively a BC break, and while it is an internal method, it should be documented in UPGRADE.md

Ocramius · 2017-06-19T01:37:35Z

lib/Doctrine/ORM/Configuration.php

@@ -541,10 +533,6 @@ public function setCustomNumericFunctions(array $functions)
     */
    public function addCustomDatetimeFunction($name, $className)
    {
-        if (Query\Parser::isInternalFunction($name)) {


@throws in docblock needs to be removed

Ocramius · 2017-06-19T01:37:39Z

lib/Doctrine/ORM/Configuration.php

@@ -483,10 +479,6 @@ public function setCustomStringFunctions(array $functions)
     */
    public function addCustomNumericFunction($name, $className)
    {
-        if (Query\Parser::isInternalFunction($name)) {


@throws in docblock needs to be removed

Ocramius · 2017-06-19T01:37:43Z

lib/Doctrine/ORM/Configuration.php

@@ -425,10 +425,6 @@ public function ensureProductionSettings()
     */
    public function addCustomStringFunction($name, $className)
    {
-        if (Query\Parser::isInternalFunction($name)) {


@throws in docblock needs to be removed

Ocramius · 2017-06-19T01:39:23Z

lib/Doctrine/ORM/Query/AST/Functions/AvgFunction.php

+    /**
+     * @var AggregateExpression
+     */
+    public $aggregateExpression;


Can this be made private?

Ocramius · 2017-06-19T01:42:29Z

lib/Doctrine/ORM/Query/AST/Functions/SumFunction.php

+    /**
+     * @inheritDoc
+     */
+    public function parse(Parser $parser)


Ocramius · 2017-06-19T01:43:03Z

lib/Doctrine/ORM/Query/Parser.php

-     *
-     * @return bool
-     */
-    static public function isInternalFunction($functionName)


This method removal is potentially a BC break, and it needs to be documented in UPGRADE.md even though this is internal API

Ocramius · 2017-06-19T01:44:32Z

lib/Doctrine/ORM/Query/SqlWalker.php

@@ -1579,10 +1579,14 @@ public function walkSimpleSelectExpression($simpleSelectExpression)
                $sql .= $this->walkPathExpression($expr);
                break;

-            case ($expr instanceof AST\AggregateExpression):
+            case ($expr instanceof AST\Functions\AvgFunction):


Since custom expressions are allowed, why not simply checking against $expr instanceof FunctionNode? Any reason why that wasn't done?

Ocramius · 2017-06-19T01:46:14Z

tests/Doctrine/Tests/ORM/Functional/CustomFunctionsTest.php

+     */
+    private $aggregateExpression;
+
+    public function parse(Parser $parser)


Add return hints

Ocramius · 2017-06-19T01:46:17Z

tests/Doctrine/Tests/ORM/Functional/CustomFunctionsTest.php

+        $this->aggregateExpression = $parser->AggregateExpression();
+    }
+
+    public function getSql(SqlWalker $sqlWalker)


Add return hints

ThePixelDeveloper · 2017-06-19T09:09:02Z

@Ocramius Think I've covered everything, although are you sure you want to have void return types? Just making sure. Tests are failing because of that reason alone.

lcobucci · 2017-06-19T09:24:44Z

@ThePixelDeveloper @Ocramius tests are failing because these changes depends on #6507. I'll review your PR now 😉

lcobucci

Besides that it LGTM

lcobucci · 2017-06-19T09:36:09Z

lib/Doctrine/ORM/Query/AST/Functions/AvgFunction.php

+ * @since   2.6
+ * @author  Mathew Davies <thepixeldeveloper@icloud.com>
+ */
+class AvgFunction extends FunctionNode
