Numerical sorting #226

dantleech · 2014-12-01T17:09:33Z

This PR adds an extra column numerical_props which is populated with LONG, DECIMAL and FLOAT values (in an XML doc).

When sorting the query will ORDER first by casting the value of the property in the numberical_props column and then by the standard props column.

SELECT ... WHERE ...  ORDER BY 
CAST(EXTRACTVALUE(n0.numerical_props, '//sv:property[@sv:name="sulu:order"]/sv:value[1]') AS DECIMAL),
EXTRACTVALUE(n0.props, '//sv:property[@sv:name="sulu:order"]/sv:value[1]')

Benchmarking

The performance should be the same when no ORDER BY is issued.
We should expect a small performance penalty when ORDER BY is issued.

Master:

SELECT \* FROM [nt:unstructured];
2143 rows in set (1.91051 sec)
2143 rows in set (1.96289 sec)
2143 rows in set (1.87208 sec)
SELECT \* FROM [nt:unstructured] ORDER BY [i18n:de-title];
2143 rows in set (1.99825 sec)
2143 rows in set (1.99817 sec)
2143 rows in set (2.02555 sec)
SELECT \* FROM [nt:unstructured] AS a WHERE i18n:de-title IS NOT NULL ORDER BY [i18n:de-title];
330 rows in set (1.63120 sec)
330 rows in set (1.63655 sec)
330 rows in set (1.65118 sec)
SELECT \* FROM [nt:unstructured] AS a WHERE ISDESCENDANTNODE(a, '/cmf/stanton/contents/service') AND i18n:de-title IS NOT NULL ORDER BY [sulu:order], [i18n:de-title] DESC;
57 rows in set (0.58143 sec)
57 rows in set (0.58995 sec)
57 rows in set (0.59365 sec)
SELECT \* FROM [nt:unstructured] AS a WHERE ISDESCENDANTNODE(a, '/cmf/stanton/contents/service');
57 rows in set (0.51940 sec)
57 rows in set (0.51984 sec)
57 rows in set (0.49899 sec)
SELECT \* FROM [nt:unstructured] AS a WHERE ISDESCENDANTNODE(a, '/cmf/stanton/contents/service') ORDER BY [i18n:de-title];
57 rows in set (0.53426 sec)
57 rows in set (0.51620 sec)
57 rows in set (0.53260 sec)
SELECT \* FROM [nt:unstructured] AS a WHERE ISDESCENDANTNODE(a, '/cmf/stanton/contents/service') AND i18n:de-title IS NOT NULL ORDER BY [i18n:de-title];
57 rows in set (0.54159 sec)
57 rows in set (0.55744 sec)
57 rows in set (0.55175 sec)
SELECT \* FROM [nt:unstructured] AS a WHERE ISDESCENDANTNODE(a, '/cmf/stanton/contents/service') AND i18n:de-title IS NOT NULL ORDER BY [sulu:order], [i18n:de-title] DESC;
57 rows in set (0.55607 sec)
57 rows in set (0.59970 sec)
57 rows in set (0.58270 sec)

This PR:

SELECT \* FROM [nt:unstructured];
2143 rows in set (1.84274 sec)
2143 rows in set (1.94709 sec)
2143 rows in set (1.86058 sec)
SELECT \* FROM [nt:unstructured] ORDER BY [i18n:de-title];
2143 rows in set (2.04875 sec)
2143 rows in set (2.04055 sec)
2143 rows in set (2.00308 sec)
SELECT \* FROM [nt:unstructured] AS a WHERE i18n:de-title IS NOT NULL ORDER BY [i18n:de-title];
330 rows in set (1.63383 sec)
330 rows in set (1.63438 sec)
330 rows in set (1.63745 sec)
SELECT \* FROM [nt:unstructured] AS a WHERE ISDESCENDANTNODE(a, '/cmf/stanton/contents/service') AND i18n:de-title IS NOT NULL ORDER BY [sulu:order], [i18n:de-title] DESC;
57 rows in set (0.57677 sec)
57 rows in set (0.60930 sec)
57 rows in set (0.58204 sec)
SELECT \* FROM [nt:unstructured] AS a WHERE ISDESCENDANTNODE(a, '/cmf/stanton/contents/service');
57 rows in set (0.48024 sec)
57 rows in set (0.47330 sec)
57 rows in set (0.48214 sec)
SELECT \* FROM [nt:unstructured] AS a WHERE ISDESCENDANTNODE(a, '/cmf/stanton/contents/service') ORDER BY [i18n:de-title];
57 rows in set (0.52143 sec)
57 rows in set (0.52957 sec)
57 rows in set (0.52393 sec)
SELECT \* FROM [nt:unstructured] AS a WHERE ISDESCENDANTNODE(a, '/cmf/stanton/contents/service') AND i18n:de-title IS NOT NULL ORDER BY [i18n:de-title];
57 rows in set (0.54349 sec)
57 rows in set (0.55270 sec)
57 rows in set (0.54978 sec)
SELECT \* FROM [nt:unstructured] AS a WHERE ISDESCENDANTNODE(a, '/cmf/stanton/contents/service') AND i18n:de-title IS NOT NULL ORDER BY [sulu:order], [i18n:de-title] DESC;
57 rows in set (0.56815 sec)
57 rows in set (0.58603 sec)
57 rows in set (0.55674 sec)

dantleech · 2014-12-01T17:10:35Z

/cc @wachterjohannes

dantleech · 2014-12-02T08:25:06Z

src/Jackalope/Transport/DoctrineDBAL/Client.php

@@ -837,70 +845,85 @@ private function syncReferences()
        }
    }

-    public static function xmlToProps($xml, ValueConverter $valueConverter, $filter = null)
+    public static function xmlToProps($xmlData, ValueConverter $valueConverter, $filter = null)


We split the properties into 3 separate DOM documents, for LONG, DECIMAL and STRING

dbu · 2014-12-02T08:44:04Z

wow, pretty involved. i wonder if we should not use http://www.postgresql.org/docs/9.3/static/datatype-json.html or something similar, if we have to go away from simply serializing to xml anyways. the json field should be efficient for search - and it might better allow for mixed data (unless i did not grasp the full issue).

dantleech · 2014-12-02T08:54:23Z

@dbu would storing in JSON hold any advantages here? (from what I can gather it might), but I don't think JSON column types are supported in most versions of MySQL.

dbu · 2014-12-02T09:00:17Z

hm, indeed mysql might be a problem here. and sqlite, and old postgres versions.

just wondering if there is something simpler than xml if all we store is just a "hashmap" with a known value format. but indeed, xml works and anything else opens a new can of worms.

dantleech · 2014-12-02T09:07:10Z

The other option would be to have a separate table for the properties, which is something similar to what EZ publish does I think. But then thats basically a different transport.

dbu · 2014-12-02T09:12:31Z

and we end up with EAV with its performance problems.

dantleech · 2014-12-02T16:00:11Z

Maybe it could be an option if we adopt a more modular architecture in Jackalope "2.0".

dantleech · 2014-12-04T07:05:50Z

@dbu @lsmith77 would you have any objections to this solution assuming that performance would not be affected? i.e. shall I continue?

lsmith77 · 2014-12-04T10:08:02Z

I am generally open to this approach. Performance is important and also the behavior of Jackrabbit in this case.

dantleech · 2014-12-05T08:13:36Z

Updated. Query tests are failing locally because of query parsing issues.. which I don't think are related.

Next I will run this through @lsmith77 s benchmarking script and see what happens.

dantleech · 2014-12-05T08:14:17Z

src/Jackalope/Transport/DoctrineDBAL/Client.php

@@ -207,6 +207,10 @@ public function __construct(FactoryInterface $factory, Connection $conn)
    private function registerSqliteFunctions(PDOConnection $sqliteConnection)
    {
        $sqliteConnection->sqliteCreateFunction('EXTRACTVALUE', function ($string, $expression) {
+            if (null === $string) {


numerical_prop can be nullable.

dantleech · 2014-12-10T14:52:45Z

Have updated and added some performance stats. Basically it is expensive enough to be a problem at the moment. Although, the MySQL perf. penalties ~0.02s > ~0.04s whilst the roundtrip penalties are maybe 5x greater, so I expect that we can do some code optimization.

That said -- the penalty is always going to be significant. Shall I make this an option?

lsmith77 · 2014-12-10T14:54:36Z

yeah .. I think it will need to be an option, ideally changeable at runtime.

dantleech · 2014-12-10T14:56:08Z

src/Jackalope/Transport/DoctrineDBAL/Client.php

+        if (!empty($xmlData['long_props'])) {
+            $xmlFields[] = $xmlData['long_props'];
+        }
+        if (!empty($xmlData['decimal_props'])) {


I guess we do not actually need separate columns here.. we can just have a numberical_props column cast to decimal regardless.

dantleech · 2014-12-10T15:17:28Z

@lsmith77 at one level we need to know at code-time because we need to persistently populate the number column, or I guess we could always populate that column. A runtime option could be used to toggle if we select from / order by it.

We could also always store all the value in props and copy the numerical values into the other column. Not very efficient, but if its an option than I guess its not too important.

lsmith77 · 2014-12-10T15:24:18Z

will we only populate those properties when the values are actually numeric? but yeah I think its ok to always populate them.

lsmith77 · 2014-12-11T12:28:52Z

tests/benchmark/resmaster.txt

@@ -0,0 +1,32 @@
+SELECT \* FROM [nt:unstructured];


what is the purpose of committing this file?

everything in this folder is an accidental commit.

dbu · 2014-12-28T08:39:26Z

restarted the build. but the errors are only about query parsing tests.

@dantleech can you squash the commits? i would say this is ready to merge now unless you know of something we missed.

dantleech · 2014-12-28T09:53:34Z

src/Jackalope/Transport/DoctrineDBAL/Client.php

-            $propsData = array('dom' => $dom);
+            if ($row['numerical_props']) {
+                $numericalDom = new \DOMDocument('1.0', 'UTF-8');
+                $numericalDom->loadXML($row['props']);


This looks wrong.

dantleech · 2014-12-28T10:18:58Z

ok.. tests are fine locally after rebasing and I fixed an issue and added a test when copying nodes. Lets see what happens with travis.

dantleech · 2014-12-28T11:12:19Z

src/Jackalope/Transport/DoctrineDBAL/Client.php

@@ -1055,7 +1120,6 @@ public function getNode($path)
            $query = '
              SELECT * FROM phpcr_nodes
              WHERE path = :path
-                AND workspace_name = :workspace


dantleech · 2014-12-28T12:02:31Z

ok! The tests are fine now -- the failing postgres test corresponds to the existing failing test in master.

dbu · 2014-12-29T10:56:39Z

tests/benchmark.back/resmaster.txt

@@ -0,0 +1,32 @@
+SELECT \* FROM [nt:unstructured];


benchmark.back - did you accidentally commit this?

dbu · 2014-12-29T10:57:48Z

apart from the benchmark.back folder, this looks good to me. @lsmith77 can you also do a final review?

dantleech · 2014-12-29T12:46:38Z

tests/Jackalope/Transport/DoctrineDBAL/Query/QOMWalkerTest.php

@@ -48,7 +48,7 @@ public function testDefaultQuery()
        $this->nodeTypeManager->expects($this->once())->method('getSubtypes')->will($this->returnValue( array() ));

        $query = $this->factory->createQuery($this->factory->selector('nt:unstructured', 'nt:unstructured'), null, array(), array());
-        list($selectors, $selectorAliases, $sql) = $this->walker->walkQOMQuery($query);
+        list($telectors, $selectorAliases, $sql) = $this->walker->walkQOMQuery($query);


- New column "numerical_props" which is populated by numerical properies and which is used exlusively for ordering.

dantleech · 2014-12-29T12:49:24Z

Removed the accidentally comitted benchmark files.

lsmith77 · 2014-12-29T13:14:21Z

wondering if we could make the dbal:init command smart enough to detect if the schema needs to be updated. or maybe we should have a specific command for this? then again .. for now I guess we will just need to provide alter statements and data migration for this change.

dantleech · 2014-12-30T11:18:22Z

I think that it makes sense that init:dbal should migate the DB (e.g. if the schema has changed we should check with the user and ask them to specify --force).

But we could do that in another PR I think.

dantleech · 2014-12-30T11:19:55Z

Created #237

dantleech · 2015-01-03T09:58:37Z

@lsmith77 @dbu what do you think?

dbu · 2015-01-04T16:42:33Z

the travis fail is only the postgres problem. +1 to merge from me.

(and maybe we should skip the failing test when the db is postgres and the env travis, if we can't find the problem, to have a reliable build)

Numerical sorting

lsmith77 added the in progress label Dec 1, 2014

dantleech force-pushed the numberical_sorting branch from 1544ded to 941b684 Compare December 2, 2014 08:23

dantleech reviewed Dec 2, 2014
View reviewed changes

dantleech reviewed Dec 5, 2014
View reviewed changes

dbu mentioned this pull request Dec 5, 2014

fix handling of numeric values for SQLite in comparisons #207

Closed

danrot mentioned this pull request Dec 9, 2014

Order smart content entries correctly in doctrine dbal sulu/sulu#638

Closed

dantleech reviewed Dec 10, 2014
View reviewed changes

dantleech changed the title ~~WIP allow sorting by integers~~ WIP allow sorting by numbers Dec 11, 2014

lsmith77 reviewed Dec 11, 2014
View reviewed changes

dantleech reviewed Dec 28, 2014
View reviewed changes

dantleech force-pushed the numberical_sorting branch 2 times, most recently from 433adb9 to a378a50 Compare December 28, 2014 10:18

dantleech reviewed Dec 28, 2014
View reviewed changes

dantleech force-pushed the numberical_sorting branch 3 times, most recently from 27988c9 to f8862a5 Compare December 28, 2014 11:45

dantleech changed the title ~~WIP Numerical sorting~~ Numerical sorting Dec 28, 2014

dantleech mentioned this pull request Dec 29, 2014

Copy references #236

Merged

dbu reviewed Dec 29, 2014
View reviewed changes

tests/benchmark.back/resmaster.txt

@@ -0,0 +1,32 @@

SELECT \* FROM [nt:unstructured];

Copy link

Member

dbu Dec 29, 2014

Choose a reason for hiding this comment

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

benchmark.back - did you accidentally commit this?

dantleech force-pushed the numberical_sorting branch from f8862a5 to acd30df Compare December 29, 2014 12:46

dantleech reviewed Dec 29, 2014
View reviewed changes

Allow sorting on numerical columns

38ca930

- New column "numerical_props" which is populated by numerical properies and which is used exlusively for ordering.

dantleech force-pushed the numberical_sorting branch from acd30df to 38ca930 Compare December 29, 2014 12:48

lsmith77 added a commit that referenced this pull request Jan 5, 2015

Merge pull request #226 from jackalope/numberical_sorting

93de594

Numerical sorting

lsmith77 merged commit 93de594 into master Jan 5, 2015

lsmith77 deleted the numberical_sorting branch January 5, 2015 08:40

lsmith77 removed the in progress label Jan 5, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Numerical sorting #226

Numerical sorting #226

dantleech commented Dec 1, 2014

dantleech commented Dec 1, 2014

dantleech Dec 2, 2014

dbu commented Dec 2, 2014

dantleech commented Dec 2, 2014

dbu commented Dec 2, 2014

dantleech commented Dec 2, 2014

dbu commented Dec 2, 2014

dantleech commented Dec 2, 2014

dantleech commented Dec 4, 2014

lsmith77 commented Dec 4, 2014

dantleech commented Dec 5, 2014

dantleech Dec 5, 2014

dantleech commented Dec 10, 2014

lsmith77 commented Dec 10, 2014

dantleech Dec 10, 2014

dantleech commented Dec 10, 2014

lsmith77 commented Dec 10, 2014

lsmith77 Dec 11, 2014

dantleech Dec 11, 2014

dbu commented Dec 28, 2014

dantleech Dec 28, 2014

dantleech commented Dec 28, 2014

dantleech Dec 28, 2014

dantleech commented Dec 28, 2014

dbu Dec 29, 2014

dbu commented Dec 29, 2014

dantleech Dec 29, 2014

dantleech commented Dec 29, 2014

lsmith77 commented Dec 29, 2014

dantleech commented Dec 30, 2014

dantleech commented Dec 30, 2014

dantleech commented Jan 3, 2015

dbu commented Jan 4, 2015

Numerical sorting #226

Numerical sorting #226

Conversation

dantleech commented Dec 1, 2014

Benchmarking

dantleech commented Dec 1, 2014

Choose a reason for hiding this comment

dbu commented Dec 2, 2014

dantleech commented Dec 2, 2014

dbu commented Dec 2, 2014

dantleech commented Dec 2, 2014

dbu commented Dec 2, 2014

dantleech commented Dec 2, 2014

dantleech commented Dec 4, 2014

lsmith77 commented Dec 4, 2014

dantleech commented Dec 5, 2014

Choose a reason for hiding this comment

dantleech commented Dec 10, 2014

lsmith77 commented Dec 10, 2014

Choose a reason for hiding this comment

dantleech commented Dec 10, 2014

lsmith77 commented Dec 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Dec 28, 2014

Choose a reason for hiding this comment

dantleech commented Dec 28, 2014

Choose a reason for hiding this comment

dantleech commented Dec 28, 2014

Choose a reason for hiding this comment

dbu commented Dec 29, 2014

Choose a reason for hiding this comment

dantleech commented Dec 29, 2014

lsmith77 commented Dec 29, 2014

dantleech commented Dec 30, 2014

dantleech commented Dec 30, 2014

dantleech commented Jan 3, 2015

dbu commented Jan 4, 2015