Opened 20 years ago

Closed 20 years ago

#614 closed defect (fixed)

[MapScript] Add a getNumResults() method on layerObj

Reported by: dmorissette Owned by: sgillies@…
Priority: high Milestone: 4.4 release
Component: MapScript-SWIG Version: 4.1
Severity: normal Keywords:
Cc: aryan@…

Description

Howard Butler pointed out that there is currently no way with SWIG MapScript to
tell the number of results on a layer.  The resultcache member of the layerObj
is not accessible via MapScript, the only link with the result cache that I can
see is the getResult(int i) method on the layerObj.

I would like to propose that we add a getNumResults() method on the layerObj
that returns the number of results currently in this layer's resultcache, like
we have in PHP MapScript.

Do you see any problem with adding this new method now?  Is there already
another way that should be used to access query results?

Attachments (1)

resultcachetest.py (3.9 KB ) - added by sgillies@… 20 years ago.
query result cache test cases

Download all attachments as: .zip

Change History (15)

comment:1 by dmorissette, 20 years ago

Cc: steve.lime@… added
Milestone: 4.2 release
What do you think, Steve?  
(BTW Sean is away for at least the rest of the week so we shouldn't expect
feedback from him on this now.)

comment:2 by sdlime, 20 years ago

The resultcache already contains a numresults member, but Sean is hiding it 
from SWIG. The getNumResults method sounds reasonable. This will change anyway 
if we move to actually caching features instead of indexes in the future. Do 
you want me to add to mapscript.i?

Steve

comment:3 by dmorissette, 20 years ago

Resolution: fixed
Status: newclosed
I've added getNumResults() to mapscript.i and updated the docs, but I'm not
setup to test it, so if you or Howard could test it and mark the bug verified
that would be great.

comment:4 by sgillies@…, 20 years ago

Hey, my protecting of resultcache and numresults was an accident.  I guess 
I was a bit overzealous when going through and tightening up access.

You know what I'd like to see in the future?  Have the query functions
return a result set object which would act more like an object that our
scripting languages' DBI modules return as the result of a database 
query.

   results = the_layer.queryByRect(...)
   
   for in range(results.numresults):
      result = results.getResult(i)
      ...





comment:5 by fwarmerdam, 20 years ago

Sean,

Are you suggesting that we should re-expose the resultscache to aovid breaking
existing scripts that got the numresults from there?  I have to admit that I
would prefer it to be exposed if that avoids breaking backwards compatibility.

However, that should be a deprecated practice.  We shouldn't document the direct
access to the resultscache for instance. 

On the question of the iterator, I am not sure why you see that as better than 
the index based approach.  If we moved to only supporting the interator, would
applications need to reissue the query if they want to make two passes over the
results set?

I know that some datastores can handle queries much more efficiently if the
receipt of results can be constrained to occur only as one pass serially. 
However, if we want to take this model, it will make it very hard to do some of
the things we are doing now with queries where we use the results cache index
list for subqueries for instance.  

In fact, constraining query access to one serial pass is quite at odds with the
talk we had about caching whole features in memory from query results. 

If think we may want to move to a "one serial pass" approach to queries, then
I think we should be discouraging use of a query results count in favour of
a method that doesn't depend on knowing the count in advance.  I would stick 
to the existing index based access to query results but depend on fetching 
results till you get a null, and encouraging only serial access.   

I don't think I am describing my concerns well here.  But if we are comfortable
sticking with the current concept of the query being completely executed on all
datasources at the point the query call is made, then I don't see any need
for an interator view of query results.   I think it is only really helpful
if we want to take a much more constrained view of how query result access
should occur. 

comment:6 by hobu, 20 years ago

I agree with Frank in that the resultscache should be re-exposed to support
backward compatibility, but not encouraged to be used by documentation. I had
quite a few places where I needed to change things because of the update,
although they were mostly just a nuisance.

From now on, I will use getNumResults() in conjunction with getResult.  Getting
the count from the resultscache was rather cumbersome anyway.


comment:7 by dmorissette, 20 years ago

I agree with Frank that we shouldn't add the cursor idea unless we change the
query mechanism to allow _not_ performing the query until we start fetching the
result for data sources where that is most efficient (i.e. the "one serial pass
idea").

However, I don't see this changing soon, or anyway if we want to be able to
release 4.4 before the June meeting we shouldn't undertake that now... so in the
meantime I second Howard's comments that getNumResults()/getResults() is a
cleaner interface and we should stick to it for now.

If/when we change the way the queries work it will be an important enough change
that we can justify changing the API (e.g. the call to open() won't be required
anymore, etc.)

comment:8 by sgillies@…, 20 years ago

Cc: aryan@… added
Resolution: fixed
Status: closedreopened
Adam Ryan pointed out to me that the resultcache was never exposed again.
His use is to get at the bounds member of the resultcache.  I really think
that the result cache should be exposed someway other than going only through
the layer.  This API:

    resultcache.numresults   # length of results
    resultcache.getResult(i) # get result at i
    resultcache.bounds       # a rectObj

is nice and concise.  Pushing everything up into layerObj like

    layer.getNumResults()
    layer.getResult()
    layer.getResultsBounds() # not implemented, but yuck!

is making layerObj unnecessarily bloated.  I'm not saying that these
methods be removed from layerObj ... but that we should go through
the resultcache.

If you don't like accessing the resultcache as an attribute, why don't
we implement a layerObj::getResults method for 4.3/4.4?

    results = layer.getResults()  # returns layer->resultcache
    layer.open()
    for i in range(results.numresults):
        shape = results.getResult(i)
        ...
    layer.close()

 

by sgillies@…, 20 years ago

Attachment: resultcachetest.py added

query result cache test cases

comment:9 by dmorissette, 20 years ago

Not sure what's best. I would be tempted to go with layer.getResultsBounds() for
simplicity, but I agree that all those methods should have been on a resultCache
object in the first place.

> If you don't like accessing the resultcache as an attribute, ...

I don't think the original problem was with exposing the resultcache as a layer
attribute: I think it was that all members of the object were visible and should
not be. So I think it would be OK to expose resultcache as an attribute as long
as you don't expose the whole thing. What you proposed seems like a good compromise:

    layer.resultcache.numresults   # length of results
    layer.resultcache.getResult(i) # get result at i
    layer.resultcache.bounds       # a rectObj

comment:10 by sgillies@…, 20 years ago

Milestone: 4.2 release4.4 release
Great.  I'm also planning to include a querying HowTo document with the
4.4 release that fully explains querying in 4.4 and what is available
for backwards compatibility.

For 4.4:
   layer.queryByPoint()
   results = layer.getResults()  # returns resultCacheObj
   results.bounds                # rectObj
   results.numresults            # int
   result = results.getResult(0) # returns resultCacheMemberObj

backwards compatibility with 4.2:
   layer.getNumResults()         # int
   result = layer.getResult(0)   # retuns resultCacheMemberObj

backwards compatibility with 4.0:
   layer.resultcache.numresults  # int
   layer.resultcache.bounds      # rectObj
   result = layer.getResult(0)   # resultCacheMemberObj
          

comment:11 by dmorissette, 20 years ago

But do you really need layer.getResults()? Why not just expose the
resultCacheObj as a member of layerObj?

comment:12 by sgillies@…, 20 years ago

Prior to a query, the result cache is NULL, and you can't test its numresults
member.  To further complicate things, SWIG wraps this NULL in a pointer
to resultCacheObj so that resultcache evaluates to an object even when it is
really NULL.  This motivated the layerObj::getNumResults() method, which 
returns 0 when the result cache is NULL.  

This is easily improved.  getResults returns NULL if the resultcache is NULL,
allowing this very clean and simple programming style A):

    layer.query()
    results = layer.getResults()
    if results:
        ... # get result members 

In the 4.2 API, users have to evaluate the number of members in the result
cache B):

    layer.query()
    numresults = layer.getNumResults()
    if numresults > 0:
        ... # get result members

which is less clean.  A is also closer to reality -- before a query, the
result cache is *really* NULL, not a set with zero members.

I'm glad you agree that result member access should not have been in the
layer API.  Maybe we should set a target for 5.0 to deprecate the old 
way of accessing members.

comment:13 by dmorissette, 20 years ago

Summary: [MapScript] Add a getNumResults() methodon layerObj[MapScript] Add a getNumResults() method on layerObj
Oh, I didn't realize that the resultcache member was NULL by default (I should
have checked the code before adding my comment).  So it seems that
layer.getResults() is the way to go then (and keep the old calls but mark them
as deprecated)

BTW, just looked at the subject of this bug. I think a new bug should have been
filed for this instead of reopening an old bug that was fixed in v4.2 in April
13th and was part of a release. I think we just create confusion if we keep
reopening bugs this way.

comment:14 by sgillies@…, 20 years ago

Resolution: fixed
Status: reopenedclosed
Closed.  New code in CVS as well as a querying howto under mapscript/docs.


Note: See TracTickets for help on using tickets.