looks sane. do we have a test for this? would be good, probably in the phpcr-api-tests to ensure proper and consistent working over all implementations.

we do not have a test yet ..

hmm the strange thing is that the following test case also works with master:

diff --git a/tests/06_Query/QueryResultsTest.php b/tests/06_Query/QueryResultsTest.php
index 6ee5a52..58608b5 100644
--- a/tests/06_Query/QueryResultsTest.php
+++ b/tests/06_Query/QueryResultsTest.php
@@ -142,6 +142,26 @@ class QueryResultsTest extends QueryBaseCase
         $this->assertEquals('14e18ef3-be20-4985-bee9-7bb4763b31de', $prop->getString());
     }

+    public function testEqualityOnNumberFields()
+    {
+        $query = $this->sharedFixture['qm']->createQuery('
+            SELECT data.longNumberToCompare
+            FROM [nt:unstructured] AS data
+            WHERE data.longNumberToCompare = 2
+              AND ISDESCENDANTNODE([/tests_general_base])
+            ',
+            \PHPCR\Query\QueryInterface::JCR_SQL2
+        );
+        $result = $query->execute();
+
+        $rows = array();
+        foreach ($result->getRows() as $row) {
+            $rows[] = $row;
+        }
+
+        $this->assertEquals(1, count($rows));
+    }
+
     public function testCompareNumberFields()
     {
         $query = $this->sharedFixture['qm']->createQuery('

another aspect which I am not sure on if its part of the spec but Jackrabbit does not match a string value on an integer property. So maybe when using the comparison operators we actually always have to check the type. that being said, maybe we should add an option to make this optional (in the dynamic typeing spirit of PHP) even if we add this.

but without this patch, the phpcr benchmark fails when I do the following change:

diff --git a/index.php b/index.php
index c3a7fd7..2e16c27 100644
--- a/index.php
+++ b/index.php
@@ -44,9 +44,9 @@ $stopWatch = new \Symfony\Component\Stopwatch\Stopwatch();

 if (empty($disableQuery)) {
     $qm = $session->getWorkspace()->getQueryManager();
-    $sql = "SELECT * FROM [nt:unstructured] WHERE count = '$nodeName' AND section = '1'";
+    $sql = "SELECT * FROM [nt:unstructured] WHERE count = $nodeName AND section = 1";
     $query = $qm->createQuery($sql, \PHPCR\Query\QueryInterface::JCR_SQL2);
-    $sql2 = "SELECT * FROM [nt:unstructured] WHERE count = '$nodeName' AND ISDESCENDANTNODE('$rootPath/1')";
+    $sql2 = "SELECT * FROM [nt:unstructured] WHERE count = $nodeName AND ISDESCENDANTNODE('$rootPath/1')";
     $query2 = $qm->createQuery($sql2, \PHPCR\Query\QueryInterface::JCR_SQL2);
     $sql = "SELECT * FROM [nt:unstructured] WHERE CONTAINS([nt:unstructured].md5, '".md5($nodeName)."')";
     $query3 = $qm->createQuery($sql, \PHPCR\Query\QueryInterface::JCR_SQL2);
@@ -132,10 +132,10 @@ function insertNodes(\PHPCR\SessionInterface $session, \PHPCR\NodeInterface $roo
 {
     for ($i = 1; $i <= $count; $i++) {
         $node = $root->addNode($i);
-        $node->setProperty('foo', 'bar', \PHPCR\PropertyType::STRING);
-        $node->setProperty('count', $i, \PHPCR\PropertyType::STRING);
-        $node->setProperty('section', $section, \PHPCR\PropertyType::STRING);
-        $node->setProperty('md5', md5($i), \PHPCR\PropertyType::STRING);
+        $node->setProperty('foo', 'bar');
+        $node->setProperty('count', $i);
+        $node->setProperty('section', $section);
+        $node->setProperty('md5', md5($i));
     }

     $session->save();

did you run the local test with all dbal transports?

making it optional is a problem. it destroys portability. and its these little things that will drive people nuts when moving from dbal to jackrabbit implementation. so we should better be strict i guess. we could maybe rewrite all queries to jackrabbit to relax on this - or ask the jackrabbit people if this is intentional.

no, I only ran the tests locally with SQLite but the changes I have done should only affect SQLite.

@dbu do you have any other idea here? it seems the bug only effects phpcr_benchmarks but not when I do "the same" in phpcr api tests ..

no idea really why that would be different. maybe different data or different schema. i guess sqlite does not autodiscover types like elasticsearch does, so that it would matter whether the first thing looks numeric?

@lsmith77 what is the state of this PR? i voiced some concerns about the clarity of messages, but we can move that into an issue and maybe fix later. the tests are green, should we merge this?

I was unable to create a failing test with the current behavior in phpcr-api-tests .. but in the benchmarks it was failing without this change ..

bizar. well it certainly is not wrong, just a bit more complex. shall we just merge?

should we merge this? is there a regression risk involved?

no .. I do not see any regression with my tests at least. but its irritating to not be able to reproduce the issue in a phpcr api test.

yeah that is strange. but do we merge or do you want to keep it open?

for now imho lets just keep it open until someone can figure this out .. i mean the issue is that the issue I am noticing in the phpcr benchmarks .. doesnt seem to exist inside the phpcr api .. so in effect this PR is fixing a non existent issue ..?

how does this relate to #226 ? both are about numbers in queries... /cc @dantleech

I am guessing that this could be implemented with the long_props and decimal_props fields in #226? @lsmith77 ? although still not sure if #226 is going to be viable, need to do some testing.

do we merge or close or keep this around? if we decide to not do anything, we should at least remove this from the milestone, and also close #206 to have only one issue around for this.

lets kill it ..