Merge lp:~zorba-coders/zorba/bug1082740_fn_subsequence into lp:zorba

[Juan Zacarias]

Added support for INF and -INF values in fn:subsequence function.

Added support for INF and -INF values in fn:subsequence function.

[Till Westmann (tillw)]

Looks good, but I think that it would be nice to factor out the (virtual) calls to getDoubleValue().
Not sure if that's measurable, but I don't think that it hurts to give the compiler a hint :)

review: Needs Fixing

[Chris Hillery (ceejatec)]

Other than Till's comment, I approve. I'm still a bit wary about having to add a new flag and test logic to fn:subsequence(), since that's used a lot, so if you happen to think of a way to do this without such checks that would be ideal. But, if this flag is necessary, then so be it.

review: Approve

[Chris Hillery (ceejatec)]

Oh, actually, two quick things:

1. Add a test case that would have failed before and succeeds now
2. Add a note about the fixed bug to the Changelog.

review: Needs Fixing

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug1082740_fn_subsequence-2013-04-10T00-23-52.125Z/log.html

[Zorba Build Bot (zorba-buildbot)]

The attempt to merge lp:~zorba-coders/zorba/bug1082740_fn_subsequence into lp:zorba failed. Below is the output from the failed tests.

CMake Error at /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake:275 (message):
  Validation queue job bug1082740_fn_subsequence-2013-04-10T00-23-52.125Z is
  finished. The final status was:

  1 tests did not succeed - changes not commited.

Error in read script: /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake

[Till Westmann (tillw)]

Also it seems that the expected failures in the FOTS need to be updated.

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug1082740_fn_subsequence-2013-04-15T16-55-56.486Z/log.html

[Zorba Build Bot (zorba-buildbot)]

The attempt to merge lp:~zorba-coders/zorba/bug1082740_fn_subsequence into lp:zorba failed. Below is the output from the failed tests.

CMake Error at /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake:275 (message):
  Validation queue job bug1082740_fn_subsequence-2013-04-15T16-55-56.486Z is
  finished. The final status was:

  1 tests did not succeed - changes not commited.

Error in read script: /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake

[Juan Zacarias (juan457)]

Hi regarding the issue of not using the flag for INF values.

The thing is the following whenever you ask for the value of a INF or -INF number you will get the highest positive or negative number available 9223372036854775807 or -9223372036854775807 being the case for 32 bits.

So I though of using the flags to avoid unnecessary processing whenever a INF value is present. It will add 5 checks for INF values to the code that's 4 ifs that would be present in every call of the subsequence and 1 more if the length is INF each of them being done only once. A way to avoid this flags we could use the value equivalents of the INF, but even though we will need to add 2 flag checks since there is actually a bug with the implementation and whenever there is an INF in the start position (we will be moving along the whole sequence with no need since it should return an empty sequence.) other problem would be present if the length is set to INF since if we manage a numeric value for the INF it would be doing a decreasing value operation and an extra check in the while statement.

That's why I thought of using the extra flags since besides we do 3 more flag checks we do them only once during the whole operation of subsequence and doesn't do unnecessary operations for INF values.

Let me know your opinion.

[Juan Zacarias (juan457)]

I made some modifications and the current solution uses 1 less check which will make it 3 and 4 and the numeric solution would have 0 extra flag checks but it still has the problem that with INF values it will do extra operations.

[Juan Zacarias (juan457)]

I apologize I made a mistake the numeric Value for INF and -INF is the same -9223372036854775808 (0x8000000000000000) which would make solving this issue with flags more complex than what I described.

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug1082740_fn_subsequence-2013-04-15T21-01-08.051Z/log.html

[Zorba Build Bot (zorba-buildbot)]

The attempt to merge lp:~zorba-coders/zorba/bug1082740_fn_subsequence into lp:zorba failed. Below is the output from the failed tests.

CMake Error at /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake:275 (message):
  Validation queue job bug1082740_fn_subsequence-2013-04-15T21-01-08.051Z is
  finished. The final status was:

  1 tests did not succeed - changes not commited.

Error in read script: /home/ceej/zo/testing/zorbatest/tester/TarmacLander.cmake

[Chris Hillery (ceejatec)]

I agree with your logic, Juan; thanks for spelling it out. I hadn't realized that your new flag were not checked in the inner loop of fn:subsequence(), but looking at the implementation it appears that is the case. So I have no problem with it.

Voting Approve now, although clearly the two failing FOTS tests will need to be addressed before it will merge.

review: Approve

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug1082740_fn_subsequence-2013-04-18T17-05-54.809Z/log.html

[Zorba Build Bot (zorba-buildbot)]

Validation queue job bug1082740_fn_subsequence-2013-04-18T17-05-54.809Z is finished. The final status was:

All tests succeeded!

[Zorba Build Bot (zorba-buildbot)]

Voting does not meet specified criteria. Required: Approve > 1, Disapprove < 1, Needs Fixing < 1, Pending < 1, Needs Information < 1, Resubmit < 1. Got: 1 Approve, 1 Needs Fixing.

[Till Westmann (tillw)]

review: Approve

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for merge proposal.
Log at: http://zorbatest.lambda.nu:8080/remotequeue/bug1082740_fn_subsequence-2013-04-24T06-13-54.41Z/log.html

[Zorba Build Bot (zorba-buildbot)]

Validation queue job bug1082740_fn_subsequence-2013-04-24T06-13-54.41Z is finished. The final status was:

All tests succeeded!

=== modified file 'ChangeLog'
--- ChangeLog	2013-04-17 16:30:00 +0000
+++ ChangeLog	2013-04-18 16:34:29 +0000
@@ -91,6 +91,7 @@
   * Raise XQST0046, if needed, from namespace declaration attribute inside
     direct element constructor.
   * Fixed bug #1023362 (xsi:type attribute ignored during validation)
+  * Fixed bug #1082740 (support for INF and -INF values in fn:subsequence function).  
 
 
 version 2.8

=== modified file 'src/runtime/sequences/sequences_impl.cpp'
--- src/runtime/sequences/sequences_impl.cpp	2013-04-11 23:37:12 +0000
+++ src/runtime/sequences/sequences_impl.cpp	2013-04-18 16:34:29 +0000
@@ -473,27 +473,46 @@
   store::Item_t startPosItem;
   xs_long startPos;
   store::Item_t lengthItem;
+  xs_double startPosDouble; 
+  xs_double lengthDouble;
 
   FnSubsequenceIteratorState* state;
   DEFAULT_STACK_INIT(FnSubsequenceIteratorState, state, planState);
 
   state->theIsChildReset = false;
-
+  
   CONSUME(startPosItem, 1);
+  startPosDouble = startPosItem->getDoubleValue();
+
+  //If starting position is set to +INF return empty sequence
+  if (startPosDouble.isPosInf() || startPosDouble.isNaN())
+    goto done;
+
+  //Removed startpos - 1, since if a -INF is present it overflows it to a positive number
   startPos =
-  static_cast<xs_long>(startPosItem->getDoubleValue().round().getNumber()) - 1;
+    static_cast<xs_long>(startPosDouble.round().getNumber());
 
   if (theChildren.size() == 3)
   {
     CONSUME(lengthItem, 2);
-    state->theRemaining =
-    static_cast<xs_long>(lengthItem->getDoubleValue().round().getNumber());
+    lengthDouble = lengthItem->getDoubleValue();
+    if (lengthDouble.isPosInf())
+    {
+      //if startPos is -INF and length is +INF return empty sequence because -INF + INF = NaN
+      if (startPosDouble.isNegInf())
+        goto done;
+
+      state->theRemaining = 1;
+    }
+    else
+      state->theRemaining =
+        static_cast<xs_long>(lengthDouble.round().getNumber());
   }
 
-  if (startPos < 0)
+  if (startPos < 1)
   {
     if (theChildren.size() >= 3)
-      state->theRemaining += startPos;
+      state->theRemaining += startPos - 1;
 
     startPos = 0;
   }
@@ -503,13 +522,13 @@
     goto done;
 
   // Consume and skip all input items that are before the startPos
-  for (; startPos > 0; --startPos)
+  for (; startPos > 1; --startPos)
   {
     if (!CONSUME(result, 0))
       goto done;
   }
 
-  if (theChildren.size() < 3)
+  if (theChildren.size() < 3 || lengthDouble.isPosInf())
   {
     while (CONSUME(result, 0))
     {

=== modified file 'src/runtime/spec/sequences/sequences.xml'
--- src/runtime/spec/sequences/sequences.xml	2013-03-20 01:19:04 +0000
+++ src/runtime/spec/sequences/sequences.xml	2013-04-18 16:34:29 +0000
@@ -421,6 +421,7 @@
     <zorba:member type="xs_long" name="theRemaining"
                   defaultValue="0" brief=""/>
     <zorba:member type="bool" name="theIsChildReset" defaultValue="false"/>
+    
 
   </zorba:state>
 

=== modified file 'test/fots/CMakeLists.txt'
--- test/fots/CMakeLists.txt	2013-04-18 14:41:27 +0000
+++ test/fots/CMakeLists.txt	2013-04-18 16:34:29 +0000
@@ -226,10 +226,6 @@
 EXPECTED_FOTS_FAILURE (fn-serialize serialize-xml-008 0)
 EXPECTED_FOTS_FAILURE (fn-string-length fn-string-length-22 0)
 EXPECTED_FOTS_FAILURE (fn-string-length fn-string-length-24 0)
-EXPECTED_FOTS_FAILURE (fn-subsequence cbcl-subsequence-004 0)
-EXPECTED_FOTS_FAILURE (fn-subsequence cbcl-subsequence-017 0)
-EXPECTED_FOTS_FAILURE (fn-subsequence cbcl-subsequence-018 0)
-EXPECTED_FOTS_FAILURE (fn-subsequence cbcl-subsequence-024 0)
 EXPECTED_FOTS_FAILURE (fn-tokenize fn-tokenize-31 0)
 EXPECTED_FOTS_FAILURE (fn-tokenize fn-tokenize-32 0)
 EXPECTED_FOTS_FAILURE (fn-unparsed-text fn-unparsed-text-037 0)

=== added file 'test/rbkt/ExpQueryResults/zorba/sequences/subsequence.xml.res'
--- test/rbkt/ExpQueryResults/zorba/sequences/subsequence.xml.res	1970-01-01 00:00:00 +0000
+++ test/rbkt/ExpQueryResults/zorba/sequences/subsequence.xml.res	2013-04-18 16:34:29 +0000
@@ -0,0 +1,1 @@
+1 2
\ No newline at end of file

=== added file 'test/rbkt/Queries/zorba/sequences/subsequence.xq'
--- test/rbkt/Queries/zorba/sequences/subsequence.xq	1970-01-01 00:00:00 +0000
+++ test/rbkt/Queries/zorba/sequences/subsequence.xq	2013-04-18 16:34:29 +0000
@@ -0,0 +1,1 @@
+fn:subsequence(for $x in 1 to 2 return $x, 1.1, xs:double('INF'))
\ No newline at end of file