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

[Paul J. Lucas]

Added count() & skip() to public Iterator API.

Added count() & skip() to public Iterator API.

[Paul J. Lucas (paul-lucas)]

review: Approve

[Paul J. Lucas (paul-lucas)]

So this looks right to me, but it doesn't work. ExtFunctionCallIterator::count() never gets called it seems. I did a test like:

let $conn := ftp:connect( "127.0.0.1", $options )
let $remote-path := "/"
return fn:count( ftp:list( $conn, $remote-path ) )

That should work, right?

[Matthias Brantner (matthias-brantner)]

The problem is that there is a TreatIterator put in place checking the return type of the function.

<FnCountIterator id="0x1926410">
  <TreatIterator type="[JSONXQType object]" quant="*" id="0x191d1e0">
    <ExtFunctionCallIterator id="0x193e380">
      <TreatIterator type="xs:anyURI" quant="" id="0x191cf20">
        <ExtFunctionCallIterator id="0x191e490">
          <SingletonIterator value="xs:string(127.0.0.1)" id="0x1927400"/>
          <JSONObjectIterator id="0x19264f0"/>
        </ExtFunctionCallIterator>
      </TreatIterator>
      <SingletonIterator value="xs:string(/)" id="0x19730c0"/>
    </ExtFunctionCallIterator>
  </TreatIterator>
</FnCountIterator>

The TreatIterator doesn't override count and, hence, the count call never gets executed on the ExtFunctionCallIterator. Could you please look at the implementation of the TreatIterator. I don't think this can easily be done but maybe you have an idea.

[Paul J. Lucas (paul-lucas)]

It depends on what the semantics of fn:count() are, i.e., is is supposed to do the type checks that TreatIterator::nextImpl() does? If the point of the count optimization is not to materialize the objects, then you (obviously) can't do type checks. But is that legal?

However, I think (correct me if I'm wrong) that the presence of fn:count() wrapped around an expression changes what the expected type is (to xs:integer) and it also makes the quantization checks irrelevant since fn:count() works on any length sequence.

If that's true, then TreatIterator::count() would simply call its child's count without any other checks. I think the same would be true for skip().

[lp:~zorba-coders/zorba/pub_iter_imp]

11691. By Paul J. Lucas on 2014-01-16

Intermedaite check-in.

11692. By Paul J. Lucas on 2014-01-16

Merge from trunk.

[Paul J. Lucas (paul-lucas)]

I've implemented TreatIterator::count(), but it's not clear what skip() should do, exactly. Should it just call its' child's skip()? Should it do some other checks? If so, what?

The problem is that skip() only reports whether there are *more* left, not how many are left.

What if the sequence has 5 elements, you tell it to skip 4, and the quantifier is 1? Is that invalid (because there were originally 5) or valid (because there is now exactly 1 left)?

(You can construct similar "what if" questions for the other quantifiers as well.)

[lp:~zorba-coders/zorba/pub_iter_imp]

11693. By Paul J. Lucas on 2014-01-17

Added TreatIterator::count() and skip().

11694. By Paul J. Lucas on 2014-01-17

Merge from trunk.

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for the following merge proposals:
https://code.launchpad.net/~zorba-coders/zorba/pub_iter_imp/+merge/201708

Progress dashboard at http://jenkins.lambda.nu/view/ValidationQueue

[Zorba Build Bot (zorba-buildbot)]

Voting criteria failed for the following merge proposals:

https://code.launchpad.net/~zorba-coders/zorba/pub_iter_imp/+merge/201708 :
Votes: {'Approve': 1, 'Pending': 1}

[Zorba Build Bot (zorba-buildbot)]

Validation queue result for https://code.launchpad.net/~zorba-coders/zorba/pub_iter_imp/+merge/201708

Stage "CommitZorba" failed.

Check console output at http://jenkins.lambda.nu:8180/job/CommitZorba/277/console to view the results.

[Matthias Brantner (matthias-brantner)]

Why are there plain english error messages in there? For example, "sequence of more than one item".

review: Approve

[Zorba Build Bot (zorba-buildbot)]

Validation queue starting for the following merge proposals:
https://code.launchpad.net/~zorba-coders/zorba/pub_iter_imp/+merge/201708

Progress dashboard at http://jenkins.lambda.nu/view/ValidationQueue

[Zorba Build Bot (zorba-buildbot)]

Validation queue succeeded - proposal merged!

[Paul J. Lucas (paul-lucas)]

> Why are there plain english error messages in there? For example, "sequence of
> more than one item".

Because the original code had them. The raiseError() function is non-trivial and I didn't feel like figuring it out at this point. It should be fixed, I agree.

=== modified file 'include/zorba/function.h'
--- include/zorba/function.h	2013-08-23 11:15:04 +0000
+++ include/zorba/function.h	2014-01-17 20:08:57 +0000
@@ -4,7 +4,7 @@
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- * 
+ *
  * http://www.apache.org/licenses/LICENSE-2.0
  *
  * Unless required by applicable law or agreed to in writing, software
@@ -30,10 +30,10 @@
 
 /**************************************************************************//**
   The Function class represents a function that is callable from XQuery code,
-  and it gives access to the various properties that are specified in the 
+  and it gives access to the various properties that are specified in the
   declaration of the function within a Prolog.
 
-  Instances of Function are returned by the StaticContext::findFunctions() 
+  Instances of Function are returned by the StaticContext::findFunctions()
   method. To be mopre precise, StaticContext::findFunctions() returns smart
   pointers to Function objects. These smart pointers must be destroyed before
   the StaticContext object they were obtained from is destroyed.
@@ -45,31 +45,27 @@
 class ZORBA_DLL_PUBLIC Function : public SmartObject
 {
  public:
-  /** \brief Destructor
-   */
-  virtual ~Function() {}
-
   /**
    * @return True if the function is sequential; false otherwise.
-   */  
+   */
   virtual bool
   isSequential() const = 0;
 
   /**
    * @return True if the function is updating; false otherwise.
-   */  
+   */
   virtual bool
   isUpdating() const = 0;
 
   /**
    * @return True if the function is private; false otherwise.
-   */  
+   */
   virtual bool
   isPrivate() const = 0;
 
   /**
    * @return True if the function is deterministic; false otherwise.
-   */  
+   */
   virtual bool
   isDeterministic() const = 0;
 
@@ -99,7 +95,7 @@
 
   /**
    * @return The arity of the function. If the function is variadic (which is
-   *         possible only for builtin functions), the result of this method 
+   *         possible only for builtin functions), the result of this method
    *         is non-deterministic.
    */
   virtual size_t
@@ -119,7 +115,7 @@
 
   /**
    * @return True if the function implementation is written in XQuery (or
-   *         equivalently, it is a non-external function with a Prolog 
+   *         equivalently, it is a non-external function with a Prolog
    *         declaration); false otherwise
    */
   virtual bool
@@ -138,10 +134,10 @@
   The ExternalFunction class serves as the base of subclasses that represent
   the implementation/body of external functions.
 
-  Instances of ExternalFunction must provide an evaluate method that serves 
-  as the implementation of the function. During its evaluation, an external 
-  function may or may not need to access the static and/or dynamic context of 
-  the invoking XQuery module. If the function implementation does need to 
+  Instances of ExternalFunction must provide an evaluate method that serves
+  as the implementation of the function. During its evaluation, an external
+  function may or may not need to access the static and/or dynamic context of
+  the invoking XQuery module. If the function implementation does need to
   access either context, the function is referred to as "contextual"; otherwise,
   it is "non-contextual".
 *******************************************************************************/
@@ -150,8 +146,7 @@
 public:
   typedef std::vector<ItemSequence*> Arguments_t;
 
- public:
-  virtual ~ExternalFunction() {}
+  virtual ~ExternalFunction();
 
   /**
    * @return The namespace URI of the function QName
@@ -189,8 +184,6 @@
 class ZORBA_DLL_PUBLIC NonContextualExternalFunction : public ExternalFunction
 {
  public:
-  virtual ~NonContextualExternalFunction() {}
-
   virtual ItemSequence_t
   evaluate(const Arguments_t&) const = 0;
 
@@ -201,7 +194,7 @@
 
 /**************************************************************************//**
   The ContextualExternalFunction class serves as the base of subclasses that
-  represent the implementation of contextual external functions. 
+  represent the implementation of contextual external functions.
 
   For each external function, an application must provide a concrete subclass
   of this class and "store" an instance of the subclass inside an ExternalModule
@@ -211,14 +204,12 @@
 class ZORBA_DLL_PUBLIC ContextualExternalFunction : public ExternalFunction
 {
  public:
-  virtual ~ContextualExternalFunction() {}
-
   virtual ItemSequence_t
   evaluate(
       const Arguments_t&,
       const StaticContext*,
       const DynamicContext*) const = 0;
-  
+
   bool
   isContextual() const { return true; }
 };

=== modified file 'include/zorba/iterator.h'
--- include/zorba/iterator.h	2013-08-01 09:52:25 +0000
+++ include/zorba/iterator.h	2014-01-17 20:08:57 +0000
@@ -34,13 +34,10 @@
 class ZORBA_DLL_PUBLIC Iterator : virtual public SmartObject
 {
  public:
-  /** \brief Destructor
-   */
-  virtual ~Iterator() {}
-
   /** \brief Start iterating.
    *
-   * This function needs to be called before calling next() or close().
+   * This function needs to be called before calling next(), count(), skip() or
+   * close().
    * Its purpose is to create and initialize any resources that may be 
    * needed during the iteration. It should not be called again until
    * after close() has been called.
@@ -73,10 +70,30 @@
   close() = 0;
 
   /**
-   * brief Check whether the iterator is open or not
+   * \brief Check whether the iterator is open or not
    */
   virtual bool
   isOpen() const = 0;
+
+  /**
+   * Counts the number of items this iterator would have returned.
+   *
+   * @throw ZorbaException if an error occurs or the iterator has not been
+   * opened.
+   */
+  virtual int64_t
+  count();
+
+  /**
+   * Skips a number of items.
+   *
+   * @param count The number of items to skip.
+   * @return \c true only if there are more items.
+   * @throw ZorbaException if an error occurs or the iterator has not been
+   * opened.
+   */
+  virtual bool
+  skip(int64_t count);
 };
 
 

=== modified file 'modules/ftp-client/ftp-client.xq.src/ftp_functions.cpp'
--- modules/ftp-client/ftp-client.xq.src/ftp_functions.cpp	2014-01-11 01:27:47 +0000
+++ modules/ftp-client/ftp-client.xq.src/ftp_functions.cpp	2014-01-17 20:08:57 +0000
@@ -612,14 +612,18 @@
 
   // inherited from Iterator
   void close();
+  int64_t count();
   bool isOpen() const;
   bool next( Item& );
   void open();
+  bool skip( int64_t );
 
 private:
   ItemFactory *const factory_;
   istream is_;
   bool open_;
+
+  bool get_line( string* );
 };
 
 list_iterator::list_iterator( curl::streambuf *cbuf, ItemFactory *factory ) :
@@ -633,10 +637,32 @@
   open_ = false;
 }
 
+int64_t list_iterator::count() {
+  int64_t count = 0;
+  string line;
+  while ( get_line( &line ) ) {
+    struct ftpparse ftp_file;
+    if ( ftpparse( &ftp_file, line.data(), line.size() ) )
+      ++count;
+  } // while
+  return count;
+}
+
 Iterator_t list_iterator::getIterator() {
   return this;
 }
 
+bool list_iterator::get_line( string *line ) {
+  while ( getline( is_, *line ) ) {
+    if ( !line->empty() ) {
+      if ( (*line)[ line->size() - 1 ] == '\r' )
+        line->erase( line->size() - 1 );
+      return true;
+    }
+  } // while
+  return false;
+}
+
 bool list_iterator::isOpen() const {
   return open_;
 }
@@ -647,11 +673,7 @@
   static Item const size_key( factory_->createString( "size" ) );
 
   string line;
-  while ( getline( is_, line ) ) {
-    if ( line.empty() )
-      continue;
-    if ( line[ line.size() - 1 ] == '\r' )
-      line.erase( line.size() - 1 );
+  while ( get_line( &line ) ) {
     struct ftpparse ftp_file;
     if ( ftpparse( &ftp_file, line.data(), line.size() ) ) {
       vector<pair<Item,Item> > kv;
@@ -709,6 +731,17 @@
   open_ = true;
 }
 
+bool list_iterator::skip( int64_t count ) {
+  string line;
+  bool more_items = true;
+  while ( count > 0 && (more_items = get_line( &line )) ) {
+    struct ftpparse ftp_file;
+    if ( ftpparse( &ftp_file, line.data(), line.size() ) )
+      --count;
+  } // while
+  return more_items;
+}
+
 list_function::list_function( module const *m ) :
   function( m, "list" )
 {

=== modified file 'src/api/CMakeLists.txt'
--- src/api/CMakeLists.txt	2014-01-03 17:31:44 +0000
+++ src/api/CMakeLists.txt	2014-01-17 20:08:57 +0000
@@ -16,8 +16,10 @@
 
 SET(API_SRCS
     base64_util.cpp
+    external_function.cpp
     smart_ptr.cpp
     diagnostic_handler.cpp
+    iterator.cpp
     zorba.cpp
     zorbaimpl.cpp
     xqueryimpl.cpp

=== added file 'src/api/external_function.cpp'
--- src/api/external_function.cpp	1970-01-01 00:00:00 +0000
+++ src/api/external_function.cpp	2014-01-17 20:08:57 +0000
@@ -0,0 +1,30 @@
+/*
+ * Copyright 2006-2013 The FLWOR Foundation.
+ * 
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <zorba/function.h>
+
+namespace zorba {
+
+///////////////////////////////////////////////////////////////////////////////
+
+ExternalFunction::~ExternalFunction() {
+  // out-of-line since it's virtual
+}
+
+///////////////////////////////////////////////////////////////////////////////
+
+} // namespace zorba
+/* vim:set et sw=2 ts=2: */

=== added file 'src/api/iterator.cpp'
--- src/api/iterator.cpp	1970-01-01 00:00:00 +0000
+++ src/api/iterator.cpp	2014-01-17 20:08:57 +0000
@@ -0,0 +1,43 @@
+/*
+ * Copyright 2006-2013 The FLWOR Foundation.
+ * 
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <zorba/item.h>
+#include <zorba/iterator.h>
+
+namespace zorba {
+
+///////////////////////////////////////////////////////////////////////////////
+
+int64_t Iterator::count() {
+  int64_t count = 0;
+  Item item;
+  while ( next( item ) )
+    ++count;
+  return count;
+}
+
+bool Iterator::skip( int64_t count ) {
+  Item item;
+  bool more_items = true;
+  while ( count > 0 && (more_items = next( item )) )
+    --count;
+  return more_items;
+}
+
+///////////////////////////////////////////////////////////////////////////////
+
+} // namespace zorba
+/* vim:set et sw=2 ts=2: */

=== modified file 'src/runtime/base/plan_iterator.cpp'
--- src/runtime/base/plan_iterator.cpp	2013-08-05 11:54:06 +0000
+++ src/runtime/base/plan_iterator.cpp	2014-01-17 20:08:57 +0000
@@ -186,7 +186,7 @@
     ++count;
   }
 
-  STACK_PUSH(GENV_ITEMFACTORY->createInteger(result, Integer(count)), state);
+  STACK_PUSH(GENV_ITEMFACTORY->createInteger(result, xs_integer(count)), state);
   STACK_END(state);
 }
 

=== modified file 'src/runtime/core/fncall_iterator.cpp'
--- src/runtime/core/fncall_iterator.cpp	2014-01-10 22:33:46 +0000
+++ src/runtime/core/fncall_iterator.cpp	2014-01-17 20:08:57 +0000
@@ -49,8 +49,9 @@
 #include "util/string_util.h"
 
 #include "store/api/index.h"
+#include "store/api/item_factory.h"
+#include "store/api/iterator_factory.h"
 #include "store/api/store.h"
-#include "store/api/iterator_factory.h"
 #include "store/api/temp_seq.h"
 
 
@@ -859,6 +860,93 @@
   }
 }
 
+bool ExtFunctionCallIterator::count( store::Item_t &result,
+                                     PlanState &planState ) const {
+  ItemSequence_t api_seq;
+
+  ExtFunctionCallIteratorState *state;
+  DEFAULT_STACK_INIT( ExtFunctionCallIteratorState, state, planState );
+
+  try {
+    if ( theFunction->isContextual() ) {
+      ContextualExternalFunction const *const f =
+        dynamic_cast<ContextualExternalFunction const*>( theFunction );
+      ZORBA_ASSERT( f );
+      StaticContextImpl sctx(
+        theModuleSctx,
+        planState.theQuery ?
+          planState.theQuery->getRegisteredDiagnosticHandlerNoSync() :
+          nullptr
+      );
+      DynamicContextImpl dctx(
+        nullptr, planState.theGlobalDynCtx, theModuleSctx
+      );
+      api_seq = f->evaluate( state->m_extArgs, &sctx, &dctx );
+    } else {
+      NonContextualExternalFunction const *const f =
+        dynamic_cast<NonContextualExternalFunction const*>( theFunction );
+      ZORBA_ASSERT( f );
+      api_seq = f->evaluate( state->m_extArgs );
+    }
+    if ( !!api_seq ) {
+      Iterator_t api_iter( api_seq->getIterator() );
+      api_iter->open();
+      int64_t const count = api_iter->count();
+      api_iter->close();
+      GENV_ITEMFACTORY->createInteger( result, xs_integer( count ) );
+    }
+  }
+  catch ( ZorbaException &e ) {
+    set_source( e, loc );
+    throw;
+  }
+  STACK_PUSH( true, state );
+  STACK_END( state );
+}
+
+bool ExtFunctionCallIterator::skip( int64_t count,
+                                    PlanState &planState ) const {
+  ItemSequence_t api_seq;
+  bool more_items;
+
+  ExtFunctionCallIteratorState *state;
+  DEFAULT_STACK_INIT( ExtFunctionCallIteratorState, state, planState );
+
+  try {
+    if ( theFunction->isContextual() ) {
+      ContextualExternalFunction const *const f =
+        dynamic_cast<ContextualExternalFunction const*>( theFunction );
+      ZORBA_ASSERT( f );
+      StaticContextImpl sctx(
+        theModuleSctx,
+        planState.theQuery ?
+          planState.theQuery->getRegisteredDiagnosticHandlerNoSync() :
+          nullptr
+      );
+      DynamicContextImpl dctx(
+        nullptr, planState.theGlobalDynCtx, theModuleSctx
+      );
+      api_seq = f->evaluate( state->m_extArgs, &sctx, &dctx );
+    } else {
+      NonContextualExternalFunction const *const f =
+        dynamic_cast<NonContextualExternalFunction const*>( theFunction );
+      ZORBA_ASSERT( f );
+      api_seq = f->evaluate( state->m_extArgs );
+    }
+    if ( !!api_seq ) {
+      Iterator_t api_iter( api_seq->getIterator() );
+      api_iter->open();
+      more_items = api_iter->skip( count );
+      api_iter->close();
+    }
+  }
+  catch ( ZorbaException &e ) {
+    set_source( e, loc );
+    throw;
+  }
+  STACK_PUSH( more_items, state );
+  STACK_END( state );
+}
 
 bool ExtFunctionCallIterator::nextImpl(
     store::Item_t& result,
@@ -906,17 +994,12 @@
         state->theResultIter = state->theResult->getIterator();
     } // if (!theFunction->isContextual())
   }
-  catch (XQueryException& e)
-  {
-    set_source( e, loc );
-    throw;
-  }
   catch (ZorbaException& e)
   {
     set_source( e, loc );
     throw;
   }
-  catch (std::exception& e)
+  catch (std::exception const& e)
   {
     throw XQUERY_EXCEPTION(
       zerr::ZXQP0001_DYNAMIC_RUNTIME_ERROR,

=== modified file 'src/runtime/core/fncall_iterator.h'
--- src/runtime/core/fncall_iterator.h	2013-09-26 09:37:30 +0000
+++ src/runtime/core/fncall_iterator.h	2014-01-17 20:08:57 +0000
@@ -259,6 +259,9 @@
   void openImpl(PlanState& planState, uint32_t& offset);
 
   bool nextImpl(store::Item_t& result, PlanState& planState) const;
+
+  bool count(store::Item_t& result, PlanState& planState) const;
+  bool skip(int64_t count, PlanState &planState) const;
 };
 
 }

=== modified file 'src/runtime/core/sequencetypes.cpp'
--- src/runtime/core/sequencetypes.cpp	2013-09-16 09:08:27 +0000
+++ src/runtime/core/sequencetypes.cpp	2014-01-17 20:08:57 +0000
@@ -32,6 +32,8 @@
 
 #include "store/api/item_factory.h"
 
+#include "zorbatypes/integer.h"
+#include "zorbatypes/numeric_types.h"
 
 namespace zorba
 {
@@ -587,6 +589,44 @@
   ar & theQName;
 }
 
+bool TreatIterator::count( store::Item_t &result, PlanState &planState) const {
+  bool const ret_val = theChild->count( result, planState );
+  xs_integer const count( result->getIntegerValue() );
+
+  switch ( theQuantifier ) {
+    case SequenceType::QUANT_QUESTION:
+      if ( count <= numeric_consts<xs_integer>::one() )
+        break;
+      // no break;
+    case SequenceType::QUANT_ONE:
+      if ( count > numeric_consts<xs_integer>::one() )
+        raiseError("sequence of more than one item");
+      // no break;
+    case SequenceType::QUANT_PLUS:
+      if ( count == numeric_consts<xs_integer>::zero() )
+        raiseError("empty-sequence()");
+      break;
+    default:
+      // do nothing
+      break;
+  }
+  return ret_val;
+}
+
+bool TreatIterator::skip( int64_t count, PlanState &planState ) const {
+  switch ( theQuantifier ) {
+    //
+    // Given that the skip() signature is not as good as it could be (it should
+    // return a pair<int64_t,bool> indicating how many were skipped and whether
+    // there are more), it's not possible to optimize this for any quantifier
+    // other than '*' (since any number of elements is valid for '*').
+    //
+    case SequenceType::QUANT_STAR:
+      return theChild->skip( count, planState );
+    default:
+      return base_type::skip( count, planState );
+  }
+}
 
 bool TreatIterator::nextImpl(store::Item_t& result, PlanState& planState) const
 {

=== modified file 'src/runtime/core/sequencetypes.h'
--- src/runtime/core/sequencetypes.h	2013-06-13 14:54:50 +0000
+++ src/runtime/core/sequencetypes.h	2014-01-17 20:08:57 +0000
@@ -213,6 +213,7 @@
                                                PlanIteratorState> 
 {
   friend class PrinterVisitor;
+  typedef UnaryBaseIterator<TreatIterator,PlanIteratorState> base_type;
 
 private:
   xqtref_t                  theTreatType;
@@ -242,6 +243,9 @@
 
   bool nextImpl(store::Item_t& result, PlanState& aPlanState) const;
 
+  bool count(store::Item_t& result, PlanState& planState) const;
+  bool skip(int64_t count, PlanState &planState) const;
+
 protected:
   void raiseError(const zstring& valueType) const;
 };