Opened 20 years ago
Closed 20 years ago
#614 closed defect (fixed)
[MapScript] Add a getNumResults() method on layerObj
Reported by: | dmorissette | Owned by: | |
---|---|---|---|
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)
Change History (15)
comment:2 by , 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 , 20 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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 , 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 , 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 , 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 , 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 , 20 years ago
Cc: | added |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
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()
comment:9 by , 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 , 20 years ago
Milestone: | 4.2 release → 4.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 , 20 years ago
But do you really need layer.getResults()? Why not just expose the resultCacheObj as a member of layerObj?
comment:12 by , 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 , 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 , 20 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Closed. New code in CVS as well as a querying howto under mapscript/docs.
Note:
See TracTickets
for help on using tickets.