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

chore(weave): table row response includes absolute row indices #3590

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

bcsherma
Copy link
Contributor

@bcsherma bcsherma commented Feb 4, 2025

Description

Return absolute index of each row when serving rows from a table. This enables more powerful UIs for editing datasets.

Copy link
Contributor Author

bcsherma commented Feb 4, 2025

@bcsherma bcsherma marked this pull request as ready for review February 4, 2025 19:57
@bcsherma bcsherma requested a review from a team as a code owner February 4, 2025 19:57
@bcsherma bcsherma force-pushed the server-table-row-index branch 2 times, most recently from a1f7dc6 to e5c77bb Compare February 5, 2025 23:46
@@ -36,21 +36,22 @@ def make_natural_sort_table_query(
row_digests_selection = f"arraySlice({row_digests_selection}, 1 + {{{pb.add_param(offset)}: Int64}}, {{{pb.add_param(limit)}: Int64}})"

query = f"""
SELECT DISTINCT tr.digest, tr.val_dump, t.row_order
SELECT DISTINCT tr.digest, tr.val_dump, t.original_index + {{{pb.add_param(offset or 0)}: Int64}} - 1 as original_index
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the offset here is needed to get the absolute index instead of the index relative to the offset.

@bcsherma bcsherma force-pushed the server-table-row-index branch from e5c77bb to 8ac6e07 Compare February 6, 2025 01:04
@bcsherma bcsherma requested a review from tssweeney February 6, 2025 17:10
Copy link
Collaborator

@tssweeney tssweeney left a comment

Choose a reason for hiding this comment

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

Please author a test suite to verify correctness

@bcsherma bcsherma force-pushed the server-table-row-index branch 3 times, most recently from 907697b to c3dc8ec Compare February 8, 2025 00:35
@bcsherma
Copy link
Contributor Author

bcsherma commented Feb 8, 2025

@tssweeney I've updated the exiting suite to test the correctness of the returned indices, which also prompted me to add the index to the response from the sqlite implementation

@bcsherma bcsherma requested a review from tssweeney February 8, 2025 01:03
@bcsherma bcsherma force-pushed the server-table-row-index branch from c3dc8ec to 99dbe7b Compare February 8, 2025 01:15
@bcsherma bcsherma merged commit 7392b81 into master Feb 8, 2025
128 checks passed
@bcsherma bcsherma deleted the server-table-row-index branch February 8, 2025 01:59
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2025
@bcsherma bcsherma restored the server-table-row-index branch February 10, 2025 22:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants