-
Notifications
You must be signed in to change notification settings - Fork 98
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
remove ordering by sys.id #507
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #507 +/- ##
==========================================
+ Coverage 87.15% 87.19% +0.03%
==========================================
Files 92 92
Lines 9834 9824 -10
Branches 1348 1344 -4
==========================================
- Hits 8571 8566 -5
+ Misses 910 907 -3
+ Partials 353 351 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying datachain-documentation with Cloudflare Pages
|
We had a lot of discussions about this through the history of dvcx / datachain. Basically it boils down to this:
Why we still end up with 1) ? ... because 2) is not really working well with billions of rows and column oriented DBs like Clickhouse and BigQuery. Having to do Anyway, I'm ok to approve this if product agrees, as this means we are removing feature which was requested explicitly by product some time ago. |
A small note: Even if we insert rows in the correct order in BQ, there is no way to guarantee the order they are returned when we pull them out. We cannot generate a sequential ID in the same way we do for SQLite and ClickHouse AND ordering is not guaranteed unless an order by statement is provided. |
8a27ee9
to
6947d0a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally! Can’t wait for this to be merged; it’ll make things simpler from the code cleanliness perspective and faster in terms of performance.
Might be controversial in terms of user experience, but on the other hand, it is kind of intuitive that you should not rely on default ordering when you’re dealing with big distributed data.
If anyone needs ordering, they can use order_by before using the related query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need sys__id at all after this decision (removing ordering)?
It is used in distributed processing. |
Related to #477 and https://github.com/iterative/studio/issues/10635#issuecomment-2381829809 & https://github.com/iterative/studio/issues/10635#issuecomment-2406225017
All the context for this change is in #477 but the tl;dr is:
This behaviour is currently undocumented, untested and misunderstood. There is no way to support this behaviour with BigQuery and we have to find another way around datasets appearing ordered. One suggestion is to save the order along with the dataset information in the metastore (perhaps save the appropriate select statement as this would be extendable in the future).