ZCatalog regression: indexing None values
Hi! Indexes shipped with ZCatalog 3 no longer work with None values: https://github.com/zopefoundation/Products.ZCatalog/commit/c378cdab2fb8997af... I'm not sure if the old behavior did follow a well defined policy, I just found this explanation: http://stackoverflow.com/a/11224076 I think in general the policy was to ignore None values if the index can't handle them. And AFAICS that's a much better policy than raising errors as ZCatalog 3 does. There is nothing wrong about None values returned by content attributes. Often they have a special meaning like not initialized, not selected, not specified. In most cases it wouldn't cause trouble if None is not indexed, but it causes trouble if indexing raises errors. Adjusting code that implements special behavior for None, adjusting persistent attributes that contain None or adding wrappers that modify indexed attributes is a lot of work. I propose to ignore None values instead of raising errors. Cheers, Yuppie
Hi, it should be said that the commit you mention clearly states the change you describe, updates the version number to make sure this won't be a patch and explains why he changed the behavior. namely to stay compatible with BTrees 4. Without looking into this more I'd say this is a required update to work With Python 3. On 05.02 12:36, yuppie wrote:
Hi!
Indexes shipped with ZCatalog 3 no longer work with None values:
https://github.com/zopefoundation/Products.ZCatalog/commit/c378cdab2fb8997af...
I'm not sure if the old behavior did follow a well defined policy, I just found this explanation:
http://stackoverflow.com/a/11224076
I think in general the policy was to ignore None values if the index can't handle them. And AFAICS that's a much better policy than raising errors as ZCatalog 3 does.
There is nothing wrong about None values returned by content attributes. Often they have a special meaning like not initialized, not selected, not specified. In most cases it wouldn't cause trouble if None is not indexed, but it causes trouble if indexing raises errors.
Adjusting code that implements special behavior for None, adjusting persistent attributes that contain None or adding wrappers that modify indexed attributes is a lot of work.
I propose to ignore None values instead of raising errors.
Cheers,
Yuppie _______________________________________________ Zope-Dev maillist - Zope-Dev@zope.org https://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - https://mail.zope.org/mailman/listinfo/zope-announce https://mail.zope.org/mailman/listinfo/zope )
Hi Patrick! Patrick Gerken wrote:
it should be said that the commit you mention clearly states the change you describe, updates the version number to make sure this won't be a patch and explains why he changed the behavior. namely to stay compatible with BTrees 4.
Correct. That's why I didn't propose to restore the old behavior.
Without looking into this more I'd say this is a required update to work With Python 3.
I'd say it is no longer possible to store None values in the index, but it's a different question if the attempt to index them is silently ignored or if an error is raised. AFAICS migrating existing code and persistent objects is much easier and in most cases unnecessary if None values are silently ignored. Cheers, Yuppie
yuppie <y.2015 <at> wcm-solutions.de> writes:
I'd say it is no longer possible to store None values in the index, but it's a different question if the attempt to index them is silently ignored or if an error is raised.
AFAICS migrating existing code and persistent objects is much easier and in most cases unnecessary if None values are silently ignored.
As the one who wrote that patch: My idea was only to raise a more meaningful TypeError from the catalog code, instead of a the very general "object has default comparision" TypeError raised by BTrees. Silently ignoring None values would also be a good approach. Or maybe one could change the 3.1.x releases to log a deprecation warning when None is indexed, and create a 4.0 release series where None is silently ignored. That way people have a chance to find the places where None is indexed and review whether or not this makes a difference in each specific case. Hanno P.S. Please cc' me on replies, I only read this mailing list every couple of weeks through the web interface.
Hi Hanno, thanks for your reply. Hanno Schlichting wrote:
yuppie <y.2015 <at> wcm-solutions.de> writes:
I'd say it is no longer possible to store None values in the index, but it's a different question if the attempt to index them is silently ignored or if an error is raised.
AFAICS migrating existing code and persistent objects is much easier and in most cases unnecessary if None values are silently ignored.
As the one who wrote that patch: My idea was only to raise a more meaningful TypeError from the catalog code, instead of a the very general "object has default comparision" TypeError raised by BTrees.
Yes. The regression was caused by upgrading to BTrees 4, not by your patch. Your patch just blessed it somehow as intended behavior.
Silently ignoring None values would also be a good approach. Or maybe one could change the 3.1.x releases to log a deprecation warning when None is indexed, and create a 4.0 release series where None is silently ignored.
That way people have a chance to find the places where None is indexed and review whether or not this makes a difference in each specific case.
Not sure if I did get you right: Products.ZCatalog 3.1 raises an error, so people will notice if None is indexed. An additional deprecation warning doesn't make sense to me. Changing the behavior would make a regression in 3.x less disruptive, so I think a 3.2 release would be fine. But I don't care much how it is called. Anyway I have no ambitions to implement the change I proposed. The issue is now in the issue tracker and I hope someone else will resolve it. Cheers, Yuppie
participants (3)
-
Hanno Schlichting -
Patrick Gerken -
yuppie