truncation refactoring and partial decimal truncation implementation #3230

jycor · 2025-09-22T20:16:53Z

changes:

reorganize more tests
fix partition test to truncate with warning
fix JSON test to match MySQL's message
partially fixes index comparison with type conversion
refactor comparison logic for IN operator
decimal truncation
add warning for negative unsigned cast

fixes:

zachmu

Overall this looks pretty good, new interfaces and methods are fine.

My main comment is that you appear to have changed the error handling semantics in calls to type.Convert() at many call sites, and this is probably pretty bad. I'm guessing this doesn't show up in practice because the current implementations don't produce errors in almost any case, but there's no guarantee that remains the case (esp with Doltgres types and future extensions). Nothing for it but to go through each call site and make sure you're keeping the same semantics for the non-truncation error cases.

zachmu · 2025-09-25T18:43:50Z

enginetest/queries/queries.go

 		Expected: []sql.Row{
-			{1, "ab"},
-			{2, "4"},
+			{1.0, "ab"},


I would call this a regression

mysql> create table types1 as select * from (values row(1+1,2+2), row(floor(1.5),concat("a","b"))) a (c,d) order by 1; Query OK, 2 rows affected (0.03 sec) Records: 2 Duplicates: 0 Warnings: 0 mysql> show create table types1; +--------+-------------------------------------------------------------------------------------------------------------------------------------------------------------+ | Table | Create Table | +--------+-------------------------------------------------------------------------------------------------------------------------------------------------------------+ | types1 | CREATE TABLE `types1` ( `c` bigint NOT NULL DEFAULT '0', `d` varchar(3) DEFAULT NULL ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci | +--------+-------------------------------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (0.00 sec)

enginetest/queries/script_queries.go

zachmu · 2025-09-25T18:54:29Z

sql/core.go


+// TruncateStringToInt trims any whitespace from s, then truncates the string to the left most characters that make
+// up a valid integer. Empty strings are converted "0". Additionally, returns a flag indicating if truncation occurred.
+func TruncateStringToInt(s string) (string, bool) {


These probably don't belong in core.go. Maybe type.go would be better.

I agree, but we unfortunately use these functions in sql.ConvertToBool and sql.EvaluateCondition, which are referenced in many, many places. sql can't import types because import cycles.

type.go in the sql package

zachmu · 2025-09-25T18:55:11Z

sql/core.go

+
+// TruncateStringToDouble trims any whitespace from s, then truncates the string to the left most characters that make
+// up a valid double. Empty strings are converted "0". Additionally, returns a flag indicating if truncation occurred.
+func TruncateStringToDouble(s string) (string, bool) {


Probably good to whip up a few basic unit tests for these, they're easy to screw up.

sql/expression/convert.go

zachmu · 2025-09-25T19:32:00Z

sql/expression/function/logarithm_test.go

-		{"Input value is valid float32", types.Float32, sql.NewRow(float32(6)), float64(1.791759469228055), nil},
-		{"Input value is valid int64", types.Int64, sql.NewRow(int64(8)), float64(2.0794415416798357), nil},
-		{"Input value is valid int32", types.Int32, sql.NewRow(int32(10)), float64(2.302585092994046), nil},
+		{"Input value is valid string", types.Float64, sql.NewRow("2"), 0.6931471805599453, nil},


These tests are fine but I would write some targeted to the new parsing functions you added directly

zachmu · 2025-09-25T19:32:56Z

sql/expression/function/math.go

-			seed = e.(int64)
-		}
+	e, _, err = types.Int64.Convert(ctx, e)
+	if err != nil && sql.ErrTruncatedIncorrect.Is(err) {


Same issue about error dropping

This applies in many other places, I'll stop mentioning it. Here the issue is that if conversion fails for any reason other than truncation, e will not necessarily be an int64 (note the err == nil handling in the original).

In other places, it seems like you're making an assumption about the error handling semantics of Convert which might be true now but are certainly not guaranteed by the interface and may not hold in the future. Gotta handle errors correctly everywhere they might occur.

zachmu · 2025-09-25T19:44:06Z

sql/expression/in.go

 			}
 		}

+		leftLit := NewLiteral(originalLeft, in.Left().Type())


This seems pointless, only used once and ever reassigned

It's used in a for loop to determine comparison types between each of the elements in the right tuple.

However, the logic should match HashInTuple, so I refactored them both here: #3237

zachmu · 2025-09-25T19:50:15Z

sql/types/number.go

 }

-func convertToInt64(t NumberTypeImpl_, v interface{}) (int64, sql.ConvertInRange, error) {
+func convertToInt64(t NumberTypeImpl_, v interface{}, round bool) (int64, sql.ConvertInRange, error) {


Would be better to define a RoundingBehavior type alias here, always irritating to have random boolean flags at the end of a method signature like this

What does this mean?

zachmu · 2025-09-25T19:52:23Z

sql/types/number_test.go

+		inRange sql.ConvertInRange
+	}{
+		// Boolean, Int8, Uint8, ... all use convertToInt64
+		{


These could use some more variety, more edge cases (leading decimal points, multiple decimal points, mix in other symbols, etc)

zachmu

LGTM

zachmu · 2025-09-29T19:24:00Z

sql/core.go

 	signIndex := 0

-	for i := 0; i < len(s); i++ {
+	var i int


This function and the constants above belong in type.go, in the same package.

Also give it a method doc while you're in there.

sql/expression/convert.go

Base automatically changed from james/truncation1 to main September 22, 2025 20:17

jycor force-pushed the james/truncation2 branch from 0d48a93 to 2908281 Compare September 22, 2025 20:28

jycor changed the title ~~more truncation refactoring~~ truncation refactoring and partial decimal truncation implementation Sep 23, 2025

jycor force-pushed the james/truncation2 branch from 19f8a82 to 2be88fd Compare September 24, 2025 18:17

jycor mentioned this pull request Sep 24, 2025

[no-release-notes] bump truncation refactor and fix some tests dolthub/dolt#9880

Merged

jycor force-pushed the james/truncation2 branch from a4d9bd8 to a94f999 Compare September 24, 2025 23:46

zachmu reviewed Sep 25, 2025

View reviewed changes

jycor force-pushed the james/truncation2 branch from 52814bb to f01c6db Compare September 26, 2025 20:39

zachmu approved these changes Sep 29, 2025

View reviewed changes

James Cor and others added 21 commits September 29, 2025 14:27

move around some tests

0d4c134

[ga-format-pr] Run ./format_repo.sh to fix formatting

7ad4127

char refactor

cd84b29

hex comparisons

3e324e5

simplify in comparison

5d69037

todos

35da24b

truncate in Convert function

71bf218

address insert tests

e82860b

[ga-format-pr] Run ./format_repo.sh to fix formatting

cf538c1

fix sql tests

c355799

convert to bool rounds

7034314

clean up

76b48cb

fix uint64 conversion

34252ae

implement rounding behavior

9ea9e08

fix some functions

50a6dce

fix tests

476dd0e

[ga-format-pr] Run ./format_repo.sh to fix formatting

12e35d3

add decimal truncation

d1a95f7

[ga-format-pr] Run ./format_repo.sh to fix formatting

65edac9

fix tests and cleanup

0515642

fix uint64

73ed063

James Cor and others added 22 commits September 29, 2025 14:27

fix test

e9c4242

skip warning tests on server engine

9c126dc

fix error message

1b47bfb

add unit tests for ConvertRound

220e05a

alter table fix

094d898

fix error for in

85e54fc

fixing up and todo

dfaf6c0

fix round and add test

892124e

fix various math functions

74a74ed

include other warnings

7744df5

fixing tests

673a418

[ga-format-pr] Run ./format_repo.sh to fix formatting

593b5fa

add more tests and don't drop error for functions

8d66cba

move code to types package

26f9764

add unit tests for truncate functions

2594b2e

migrate functions

eed303d

number tests

0b3c16b

fix ceil and floor typing

f3bcdf0

fix queries

fb06f2d

fix more queries

035f4c1

more tests

498a67b

move function and add method doc

c4ffba6

jycor force-pushed the james/truncation2 branch from f01c6db to c4ffba6 Compare September 29, 2025 21:37

[ga-format-pr] Run ./format_repo.sh to fix formatting

938c654

jycor merged commit fb75d24 into main Sep 29, 2025
7 of 8 checks passed

BrewTestBot mentioned this pull request Oct 1, 2025

dolt 1.59.14 Homebrew/homebrew-core#246514

Merged

Uh oh!

truncation refactoring and partial decimal truncation implementation #3230

truncation refactoring and partial decimal truncation implementation #3230

Uh oh!

Conversation

jycor commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zachmu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jycor commented Sep 22, 2025 •

edited

Loading