Skip to content

compare bug in temporary tables#64

Merged
tomas-villagesql merged 1 commit intomainfrom
tomas/fix-custom-type-union-compare
Feb 26, 2026
Merged

compare bug in temporary tables#64
tomas-villagesql merged 1 commit intomainfrom
tomas/fix-custom-type-union-compare

Conversation

@tomas-villagesql
Copy link
Copy Markdown
Member

No description provided.

@tomas-villagesql tomas-villagesql requested review from malone-at-work and villagestevers and removed request for villagestevers February 26, 2026 16:55
if (result.has_value()) {
return result.value();
}
return const_cast<Field *>(m_mysql_field)->key_cmp(lhs_data, rhs_data);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So the call WAS going through sql/field.cc Line 6757, which was trying to use the length prefix. But if it is NOT a custom type, it is doing the same thing below on Line 6764. Why is that not problematic? Do we have a different representation than other varstring?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't follow

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We had a discussion offline, it is because we aren't using an encoding, which is not something that other binary varchars would use. I am not sure if there is a simplification, that would get rid of these special cases, but let's unblock for now.

@tomas-villagesql tomas-villagesql force-pushed the tomas/fix-custom-type-union-compare branch from 79a1a5e to 5660f35 Compare February 26, 2026 17:38
int cmp_complex(const unsigned char *data1, size_t len1,
const unsigned char *data2, size_t len2) {
if (len1 < kComplexSize || len2 < kComplexSize) {
assert(len1 != 0);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want this, or len1 != kComplexSize and len2 as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I replace this assert in a follow up PR, with a warning, so I don't think it is worth changing here.

@tomas-villagesql tomas-villagesql merged commit 65908ea into main Feb 26, 2026
9 checks passed
@tomas-villagesql tomas-villagesql deleted the tomas/fix-custom-type-union-compare branch February 26, 2026 18:02
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 26, 2026
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.

3 participants