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

Add support for Doltgres indexes #8186

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Conversation

Hydrocharged
Copy link
Contributor

@coffeegoddd
Copy link
Contributor

@Hydrocharged DOLT

comparing_percentages
100.000000 to 100.000000
version result total
17851bb ok 5937457
version total_tests
17851bb 5937457
correctness_percentage
100.0

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the doltgresql side, I mentioned this before but I think reimplementing map iters isn't the best separation of concerns. @zachmu is better at interface tradeoffs, but I think hiding all of the details behind a different Range.Matches is better than duplicating iterators. I could be missing some bespoke postgres logic that requires more control, but it looks like binary search callbacks to me. If "findStart" / "findStop" can't be rewritten for our existing star cursor/end cursor iters, I think it'd be straightforward to add those primitives to the tree layer rather than implementing in the index package. Even if we branch logic without interface niceties, it's just such a smaller surface area to test/way greater code reusability if we keep the map iter implementations narrow. Duplicating fixes to all of the noms iters for so long was inefficient, a bit of time spent avoiding that state again would be my vote. I apologize for the handwavy nature of these comments, hopefully it makes sense from prior context, and happy to dive into more specifics if you want.

@Hydrocharged Hydrocharged force-pushed the daylon/doltgres-indexes branch 2 times, most recently from f97e2f6 to ead9df6 Compare August 22, 2024 14:00
Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it's in a place that would be amenable to refactors. It'd be nice to maybe document why there is non-logical overlap between mysql and PG ranges, in case we want to try to make the arithmetic faster at some point. The start/stop semantics don't seem bad, but it is different from how we do it in MySQL, and if I had to guess it seems like both the MySQL and PG sides could use either interface with the right compensating filters.

@Hydrocharged Hydrocharged changed the title POC for Doltgres indexes Add support for Doltgres indexes Aug 27, 2024
@Hydrocharged Hydrocharged force-pushed the daylon/doltgres-indexes branch 2 times, most recently from d345826 to 68fb61e Compare August 27, 2024 10:28
@Hydrocharged Hydrocharged marked this pull request as ready for review August 27, 2024 10:28
@Hydrocharged
Copy link
Contributor Author

dolthub/doltgresql#561

@coffeegoddd
Copy link
Contributor

@Hydrocharged DOLT

comparing_percentages
100.000000 to 100.000000
version result total
24b49d0 ok 5937457
version total_tests
24b49d0 5937457
correctness_percentage
100.0

@Hydrocharged Hydrocharged merged commit e4bb9ca into main Aug 27, 2024
20 of 21 checks passed
@Hydrocharged Hydrocharged deleted the daylon/doltgres-indexes branch August 27, 2024 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants