Skip to content

[no-release-notes] bump and fix#1548

Closed
jycor wants to merge 8 commits intomainfrom
james/hashof
Closed

[no-release-notes] bump and fix#1548
jycor wants to merge 8 commits intomainfrom
james/hashof

Conversation

@jycor
Copy link
Copy Markdown
Contributor

@jycor jycor commented Jun 13, 2025

The two regressions here are not easily fixable.

The problem lies in numeric types and conversions.
When we have a float64 on the left side and a numeric (decimal type underlying), converting causes a panic.

Later on, we compare the values using a float64 comparator; it seems like the comparator wants to use left type as the comparing type, and panics if the arguments are not strictly that type.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 13, 2025

Main PR
covering_index_scan_postgres 355.66/s 333.93/s -6.2%
index_join_postgres 155.59/s 155.10/s -0.4%
index_join_scan_postgres 186.04/s 186.93/s +0.4%
index_scan_postgres 12.43/s 12.54/s +0.8%
oltp_point_select 2514.21/s 2595.14/s +3.2%
oltp_read_only 1813.33/s 1855.69/s +2.3%
select_random_points 115.55/s 117.19/s +1.4%
select_random_ranges 130.32/s 131.19/s +0.6%
table_scan_postgres 11.76/s 11.64/s -1.1%
types_table_scan_postgres 5.45/s 5.44/s -0.2%

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 13, 2025

Main PR
Total 42090 42090
Successful 16455 16455
Failures 25635 25635
Partial Successes1 5546 5546
Main PR
Successful 39.0948% 39.0948%
Failures 60.9052% 60.9052%

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

@jycor jycor requested a review from Hydrocharged June 16, 2025 22:59
Copy link
Copy Markdown
Collaborator

@Hydrocharged Hydrocharged left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +102 to +103
// TODO: type conversions for numeric types can panic.
// TODO: this should be rType to match properly
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Rather than saying this should be the right type in the TODO, I'd change it to say that hash.HashOf expects the right type, which doesn't quite align with how we handle type casting.

@jycor jycor enabled auto-merge (squash) June 18, 2025 09:22
@zachmu zachmu closed this Jun 20, 2025
auto-merge was automatically disabled June 20, 2025 20:48

Pull request was closed

@jennifersp jennifersp deleted the james/hashof branch August 14, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants