Conversation
The split between Temptable and Offset query builders was originally introduced in github#471. Then, both query builders have been changed to calculate actual chunk sizes in github#1500. The updated implementation of the Offset query introduced a bug, where a missing `order by` clause in `select_osc_chunk` can result in end values potentially surpassing the chunk_size. This wasn't detected during our initial testing where the query just returned rows in their original order, but may happen in real-world scenarios in case the db returns data in an undefined order. An obvious fix would be to just add an `order by` to the Offset builder subquery, however since both builders use temptables now, it makes more sense to simplify and use only one of them. Alternatively, the builder could only use the less performant count query variants when `--panic-on-warnings` is enabled and otherwise use the simpler ones from pull/471. We decided to not follow this path for now, hoping `--panic-on-warnings` becomes an updated and safer default in the future. Co-authored-by: Bastian Bartmann <bastian.bartmann@shopify.com>
grodowski
commented
May 22, 2025
| from | ||
| %s.%s | ||
| where | ||
| %s and %s |
Member
Author
There was a problem hiding this comment.
order by should be added here if we were to keep the offset variant
Member
Author
There was a problem hiding this comment.
Instead, we're dropping the expectedRowCount entirely after discovering more issues with the count approach: github#1557 (comment)
coding-chimp
reviewed
Jun 4, 2025
ef735d1 to
53b9faf
Compare
coding-chimp
approved these changes
Jun 4, 2025
53b9faf to
80568cd
Compare
- Removed count subqueries from range builders to restore the more performant approach from PR github#471 - Modified panic-on-warnings logic to trigger errors based solely on SQL warnings, not on row count mismatches - This addresses potential race conditions where row count comparisons could produce false positives due to concurrent table modifications
80568cd to
d4157d7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The split between Temptable and Offset query builders was originally introduced in github#471. Then, both query builders have been changed to calculate actual chunk sizes in github#1500.
The updated implementation of the Offset query introduced a bug, where a missing
order byclause inselect_osc_chunkcan result in end values potentially surpassing the chunk_size. This wasn't detected during our initial testing where the query just returned rows in their original order, but may happen in real-world scenarios in case the db returns data in an undefined order.An obvious fix would be to just add an
order byto the Offset builder subquery,however since both builders use temptables now, it makes more sense to simplify and use only one of them.Alternatively, the builder could only use the less performant count query variants when
--panic-on-warningsis enabled and otherwise use the simpler ones from pull/471. We decided to not follow this path for now, hoping--panic-on-warningsbecomes an updated and safer default in the future.Update: following github#1557 (comment) the builders were rolled back to original and we think we can safely try a more strict
PanicOnWarningswith no row count check.script/cibuildreturns with no formatting errors, build errors or unit test errors.