Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow string ids in indexes (or at least improve the error messages) #303

Open
codeananda opened this issue Jan 24, 2024 · 2 comments
Open

Comments

@codeananda
Copy link
Contributor

I've just spent the last couple of hours being super confused by the following error message.

from rtree import index

points = (0.0, 0.0, 1.0, 1.0)
def gen():
    for s, p in [('a', points)]:
        yield s, p
        
index.Index(gen())
RTreeError: Error in "Index_CreateWithStream": IllegalArgumentException: RTree::BulkLoader::bulkLoadUsingSTR: Empty data stream given.

Improvement requests:

  • Add the ability to use string indexes (useful for databases that use things like 00be976b-691b-4e1a-bcef-9336f13fb290 as keys), or
  • Improve the error message so that it tells you you shouldn't be using string indexes.

Similarly, this error could also be improved:

idx = index.Index()
idx.insert('a', points)
ArgumentError: argument 2: <class 'TypeError'>: wrong type

Issues:

  • Should be TypeError
  • It's the first argument

Something like

TypeError(f"Id values must be integers. Received {id=} with type {type(id}}")

I'm open to implementing some of the things but I don't know C, only Python.

@codeananda codeananda changed the title Allow string ids for items (or at least improve the error messages) Allow string ids in indexes (or at least improve the error messages) Jan 24, 2024
@hobu
Copy link
Member

hobu commented Jan 29, 2024

Add the ability to use string indexes (useful for databases that use things like 00be976b-691b-4e1a-bcef-9336f13fb290 as keys), or

This would require changing the design of libspatialindex. 👎

Improve the error message so that it tells you you shouldn't be using string indexes.

We would happily merge a PR that implemented a better error message and a test that demonstrated it.

@codeananda
Copy link
Contributor Author

This would require changing the design of libspatialindex. 👎

I thought it might

We would happily merge a PR that implemented a better error message and a test that demonstrated it.

Cool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants