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

Internal comparisons consider NULL values higher than non-NULL values #1903

Open
nicktobey opened this issue Jul 26, 2023 · 0 comments
Open

Comments

@nicktobey
Copy link
Contributor

From the MySQL documentation:

Like SQLite, MySQL considers NULL values lower than any non-NULL value. If you use this database, expect the same treatment of NULL values as illustrated above: NULLs will appear first if the values are sorted in ascending order and last if descending order is used.

This is echoed by the docstring for sql/types/conversion.go::CompareNulls:

// CompareNulls compares two values, and returns true if either is null.
// The returned integer represents the ordering, with a rule that states nulls
// as being ordered before non-nulls.
func CompareNulls(a interface{}, b interface{}) (bool, int) {
	aIsNull := a == nil
	bIsNull := b == nil
	if aIsNull && bIsNull {
		return true, 0
	} else if aIsNull && !bIsNull {
		return true, 1
	} else if !aIsNull && bIsNull {
		return true, -1
	}
	return false, 0
}

However, the implementation is at odds with the documentation: nulls are compared higher than non-nulls.

I don't believe this behavior is observable: every operation that cares about comparing nulls with non-nulls does their own explicit check on the operands before calling Type::Compare.

However, the fact that we compare these values differently than MySQL is likely to be a source of confusion for developers, and should be fixed.

There is at least one code path that depends on the current behavior: AutoIncrement::Eval has different behavior if the operand is greater than 0 or less than 0, and will need a special case for a null operand when one was previously not required.

There may be other places where we depend on the current implementation. Those will need to be cleaned up.

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

No branches or pull requests

1 participant