-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Between ordering - string < date #2774
Comments
Hum, actually I just can't get any result when I'm using a date in |
The orderings in We need to make these consistent for anything to make sense. Since people probably have data on-disk indexed by a pseudotype, I think we should change |
Assigning to @Tryneus because he told me he has some free time. |
@mlucy -- I thought that pseudotypes were behaving like other types, that is to say we compare them by their name. array < bool < null < number< string < time Is there a reason a pseudo types are always greater than a normal type? Or is it just that we only have time for now? |
@neumino -- if you call |
I'm pretty sure changing |
@Tryneus: what would break? |
(I mean, besides the obvious fact that sindex functions would compare types differently after the change.) |
So, in the simplest example I can come up with, a user has a table with one entry: They add an index Before changing |
I'm honestly not sure what to think. There are basically three options:
@srh -- I assume you're strongly in favor of the second option? |
Can we not bump the version tag on the sindex and recreate it during import, rather than require the user to do it? |
@gchpaco: I think the goal is to not force people to re-import their data between minor versions any more. |
Actually, that's another option: we could leave this broken until 2.0 and fix it then. (There are obvious problems with that, though.) |
Put another way: upon startup, identify old versions of these sindexes and rebuild them quietly? Thus migrating the on-disk format to the current version but without requiring manual operations of the user. |
In this specific case that could be as easy as "old index, ok, drop it and recreate using original specs but not broken this time" |
I see, you mean re-create the sindex from scratch to match the new interpretation of the sindex function? We'd have to re-create almost all the sindexes (since so many things do comparisons), which would prevent queries on the sindexes from running, possibly for a long time, and afterward the results of some queries would be different than they were before. But maybe that's better than the other options; what do other people think? |
We could conceivably retain the old index while we rebuild it to serve queries, although that could be a bad idea from the codebase POV. |
Here's a proposal: If these changes might break an index, then we flag the index and put a warning message in the log. Queries against a flagged index fail with a message explaining the situation. The user can rebuild the index (which automatically clears the flag) or manually unflag it. If the index should have been rebuilt but the user manually unflags it, then the behavior is undefined. |
Yes. Also, the datum_t::cmp-specific work is not really a pain in the ass. It comprises some clear and specific code that will exist for a version or maybe two (or maybe five! But it's not so bad.). The harder work is in the general support of this feature. But it's important that we not crash and lose users' data, that we be capable of fixing bugs in the ReQL implementation or specification, and that users' databases don't refuse to work after an upgrade. This is the way to do it. In order to tell users they need to rebuild indexes, this is simplest and most transparent: They rebuild the new index with a different name, while the old one still exists, and then they the new index. There are no technical barriers to supporting index renaming. E.g. |
I'm working on the backwards compatibility stuff in sam_2774. I hope to get it done by today. |
Review 1847 contains disk format stuff in branch sam_2774_disk_structure. This only implements the disk format changes (including 1.13 backwards compatibility of course), and the use of the secondary index reql version found on disk. Scanning the sindex func or anything related to that is not implemented yet. |
In light of #2789, I think we may have to just force people to re-import their data this release, which would obviate this problem. |
This is fixed in |
Sorry for the necropost. But this should be somewhere in the docs, if it's not already. 😄 |
@aleclarson thanks, I created an issue in the docs repo: rethinkdb/docs#1210 |
This return
true
This return the states I expect (basically all of them) -
state
is a simple index on the fieldstate
and states are all strings.However this return nothing:
Ping @mlucy
The text was updated successfully, but these errors were encountered: