Optimize date parsing for format yyyy-MM-dd

[ubyyj]

Description

Optimize the performance of date parsing if the date is in standard format (yyyy-MM-dd).
The following functions are impacted:

date(str)
cast(str as date)
date_parse(str, '%Y-%m-%D')

After:
Benchmark Mode Cnt Score Error Units
BenchmarkParseDate.parseDate thrpt 10 762.769 ± 35.198 ops/ms

Before:
Benchmark Mode Cnt Score Error Units
BenchmarkParseDate.parseDate thrpt 10 48.887 ± 3.222 ops/ms

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`15229`)

[xumingming]

Can you share performance results?

[ubyyj]

Can you have performance results?

I benchmarked the performance on my local laptop, before the optimization, the QPS is about 3M, after optimization, it's about 45M, so basically it's 15 times faster.

[lukasz-stec]

Use OptionalInt as a return type, it's more explicit and avoids using magic Integer.MIN_VALUE as a wrong format marker.

[ubyyj]

fixed in cb70c37e453480e7b5891e117f63796e1ac450fb

[lukasz-stec]

What t stands for? Maybe rename it to digit?

[ubyyj]

thanks, fixed in cb70c37e453480e7b5891e117f63796e1ac450fb

[lukasz-stec]

how much benefit does the bit shifting brings over year = year * 10 + t?

[ubyyj]

it turned out there is no observable perf difference, so changed to year = year * 10 + t for readability in cb70c37e453480e7b5891e117f63796e1ac450fb

[lukasz-stec]

import java.time.LocalDate

[ubyyj]

fixed in cb70c37e453480e7b5891e117f63796e1ac450fb

[lukasz-stec]

should tryParseDateYyyyMmDd support a negative year? e.g. -1970-01-01
also, add more test cases: e.g. min, max date that can be parsed, year zero, 29 feb leap year etc

[ubyyj]

tryParseDateYyyyMmDd will skip format like -1970-01-01, and will fall back to the original implementation.
Anyway, I will add more test cases as you suggested

[ubyyj]

more test cases added in cb70c37e453480e7b5891e117f63796e1ac450fb

[pettyjamesm]

Why not: int year = Integer.parseInt(value, 0, 4, 10) ? Same when parsing month and day, I suspect the Integer.parseInt(CharSequence, beginIndex, endIndex, radix) overload will perform similarly or better to hand-rolling here but I could be wrong.

[ubyyj]

Integer.parseInt is about 30% slower, probably because there are more checks in that function.

[findepi]

I am surprised we can do Integer.parseInt faster than Java. is their implementation just suboptimal?
in any case, it's feels better to encapsulate this in a utility method and use that for year, month and day. The code would be much more readable.

[ubyyj]

@pettyjamesm @lukasz-stec would you please take a look again? All comments addressed.

[lukasz-stec]

How about using java.util.OptionalInt as a return type? it would make it easier for jit to optimize code (avoid boxed type allocation etc.).

[ubyyj]

thanks for the suggestion. Updated to OptionalInt in 921b9c23e9894fdc57a581a29cb5d6aee89a99f9

[lukasz-stec]

I wouldn't reuse this local variable. You can instead have something like

int monthHigh = value.charAt(5) - '0';
...
int monthLow = value.charAt(6) - '0';
...
int dayHigh = value.charAt(8) - '0';
...
int dayLow = value.charAt(9) - '0';

[ubyyj]

updated in 921b9c23e9894fdc57a581a29cb5d6aee89a99f9

[lukasz-stec]

Add an invalid case for every if statement in the tryParseDateYyyyMmDd e.g. 197A-01-01, 197#-01-01 so that all code paths are covered.

[ubyyj]

a test case added for each code path in 921b9c23e9894fdc57a581a29cb5d6aee89a99f9

[lukasz-stec]

why do we throw for this case and not for others above like 'Dec 24 2022'?

[ubyyj]

2022-02-29 has a valid format, but the value does not exist. This kind of error is hard to detect in our code, so delegate to LocalDate.of().

for case like Dec 24 2022, its format is wrong, and we can easily detect it in our code, we return OptionalInt.empty(), and delegate the behavior to the original implementation, to be backward compatible as much as possible..

[lukasz-stec]

is there a case where

        if (monthLow < 0 || monthLow > 9) {
            return OptionalInt.empty();
        }

or

    if (value.charAt(4) != '-') {
            return OptionalInt.empty();
        }

would be parsed correctly by DATE_FORMATTER.parseMillis?
Looks to me as if there is not. If that's the case, why not throw DateTimeException from tryParseDateYyyyMmDd if it's gonna fail anyway?

[ubyyj]

DATE_FORMATTER.parseMillis support format other than yyyy-MM-DD, e.g. 99-12-31 and 1970-2-01.
Lets take 1970-2-01 for example, the monthLow would be the '-', and if (monthLow < 0 || monthLow > 9) returns true, but we can't say it's a invalid format, and throw here.
So we just delegate to DATE_FORMATTER.parseMillis for format other than yyyy-MM-DD

[lukasz-stec]

You do check the length at the beginning but I guess with a possible negative year this could still be confused e.g. -1111-1-01.
Given #10677 we actually don't need to support negative years but I think it's better to separate those changes (perf improvement and backward incompatible change).

[lukasz-stec]

generally lgtm % benchmark and small comments

[lukasz-stec]

You do check the length at the beginning but I guess with a possible negative year this could still be confused e.g. -1111-1-01.
Given #10677 we actually don't need to support negative years but I think it's better to separate those changes (perf improvement and backward incompatible change).

[lukasz-stec]

Could you add a jmh benchmark for this similar to other benchmarks here e.g. io.trino.operator.scalar.BenchmarkArrayEqualOperator and post results before/after in the commit message and in the pr description?

[ubyyj]

done

[lukasz-stec]

you can move this variable inside the loop now

[ubyyj]

done in 72e06703cbffd04afc38670c2f73c497db9c748b

[lukasz-stec]

using a compile-time constant may not be a realistic benchmark (jit can do some tricks with constants).
I would suggest generating some number of dates to parse in the @setup and iterate over them if this method.

Another thing is that the result of the parseDate is ignored. This can make the compiler drop the whole computation (dead code elimination). To fix this you can use org.openjdk.jmh.infra.Blackhole to "consume" output of every parseDate

[ubyyj]

done in f19d4a7cab505376b7162119a89dc8a4169b2815

[lukasz-stec]

Is the warmup time enough to give time to jit to compile the code?

[ubyyj]

extended both warmup time and measure time, results are quite stable. Changed in f19d4a7cab505376b7162119a89dc8a4169b2815

[lukasz-stec]

One last thing to do is to clean up commits. I would put the commit that adds benchmark first and squash the rest in the second commit

[ubyyj]

hi, got some issue to reorder the commits. I touched the tryParseDateYyyyMmDd() function in the commit that adds benchmark (in 72e06703cbffd04afc38670c2f73c497db9c748b, move curDigit into the loop). If I put the commit that adds benchmark first, them there is conflicts, as that time there is no tryParseDateYyyyMmDd() yet.

So how about let's not swap the order, leave add benchmark commit the last one?

[ubyyj]

I just squashed the 6 commits into 2:

1. logic code change;
2. add benchmark

[lukasz-stec]

I just squashed the 6 commits into 2:

1. logic code change;
2. add benchmark

Small comment.
Could you move the "[Add benchmark for DateTimeUtils.parseDate]" commit so that it is first? This way one can checkout this commit and run the benchmark "before" easily.
I would also move the before/after benchmark results to the other commit message as this is the commit that introduces the "after"

[ubyyj]

ah I got your point why the order is important. Done

[lukasz-stec]

lgtm
@findepi @sopel39 this is ready for maintainer review

[findepi]

would it be correct to call it iso8861DateFormat?

[ubyyj]

updated in d6f05bd8ec6e41ee32e70f8d6cfb128e447d74b0

[findepi]

remove, reuse io.trino.spi.type.Timestamps#MILLISECONDS_PER_DAY

[ubyyj]

done in d6f05bd8ec6e41ee32e70f8d6cfb128e447d74b0

[findepi]

parseIfIso8861DateFormat?

[ubyyj]

renamed in d6f05bd8ec6e41ee32e70f8d6cfb128e447d74b0

[findepi]

I am surprised we can do Integer.parseInt faster than Java. is their implementation just suboptimal?
in any case, it's feels better to encapsulate this in a utility method and use that for year, month and day. The code would be much more readable.

[findepi]

If you also validate that it is 4 digits, then hyphen, then two digits, then hyphen, then two digits, the following code will need to further checks, and be much simpler

[ubyyj]

checking the format at the beginning was our initial implementation, but it turned out to be 25% slower, because that way we need to call String.charAt() more. We need to call it during checking and parsing.
Though we can store the chars in an array when do format checking, but that also make code a little longer, and more GC pressure, since it needs to allocate an array each time.

below is the detailed benchmark result:
// mixing format check and parsing (current implementation)
Benchmark Mode Cnt Score Error Units
BenchmarkParseDate.parseDate thrpt 10 742.748 ± 40.368 ops/ms

// check format at the begining
Benchmark Mode Cnt Score Error Units
BenchmarkParseDate.parseDate thrpt 10 561.297 ± 29.083 ops/ms

[findepi]

This method should be private

if you want to test it directly, make it package-private and annotate with @VisibleForTesting

[ubyyj]

done in d6f05bd8ec6e41ee32e70f8d6cfb128e447d74b0

[findepi]

Suggested change

	private static OptionalInt parseIntSimple(String str, int start, int len) {
	private static OptionalInt parseIntSimple(String str, int start, int len)
	{

[ubyyj]

fixed in 9db1745425af8978c7dd0cfc4f3793cb610e1a33

[findepi]

"len number of chars to parse, and must >= 1" so throwing IAE would be appropriate

[ubyyj]

let's keep this check, otherwise, for len==0, it will return OptionalInt.of(0) without throwing any exception, while OptionalInt.empty() is expected

[findepi]

You can initialize result to 0 and then you can start for loop with i=0, and have 3 fewer lines

[ubyyj]

done in 9db1745425af8978c7dd0cfc4f3793cb610e1a33

[findepi]

spell out: currentDigit (or just digit)

[ubyyj]

fixed in 9db1745425af8978c7dd0cfc4f3793cb610e1a33

[findepi]

Please follow https://github.com/airlift/codestyle to configure your IDE so that the code is well formatted.

[findepi]

Suggested change

	* parse positive integer with radix 10, with no null or length check.
	* Parse positive integer with radix 10, with no null or length check.
	*

[ubyyj]

fixed in 9db1745425af8978c7dd0cfc4f3793cb610e1a33

[findepi]

follow up airlift/airbase#338

[findepi]

Arguably this would be a bit more readable

if (value.length() != 10 || value.charAt(4) != '-' || value.charAt(7) != '-') {
    return OptionalInt.empty();
}

OptionalInt year = parseIntSimple(value, 0, 4);
if (year.isEmpty()) {
    return OptionalInt.empty();
}

OptionalInt month = parseIntSimple(value, 5, 2);
if (month.isEmpty()) {
    return OptionalInt.empty();
}

OptionalInt day = parseIntSimple(value, 8, 2);
if (day.isEmpty()) {
    return OptionalInt.empty();
}

LocalDate date = LocalDate.of(year.getAsInt(), month.getAsInt(), day.getAsInt());
return OptionalInt.of(toIntExact(date.toEpochDay()));

• dashes are checked upfront, so they don't distract flow later
• // year, // month comments removed; variables names are meanigful
• ld renamed to date. https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#avoid-abbreviations

[ubyyj]

applied in 9db1745425af8978c7dd0cfc4f3793cb610e1a33

[findepi]

• use assertThat(exp).isEmpty()
  • no need for re-assigned every time variable, each test is own independent line (or lines), exception message would include value contents if assertion fails
• you can import parseIfIso8861DateFormat statically

assertThat(parseIfIso8861DateFormat("Dec 24 2022")).isEmpty();

[ubyyj]

done in 9db1745425af8978c7dd0cfc4f3793cb610e1a33

[martint]

Did you mean ISO 8601 ?

[findepi]

ups.. thanks!

#15756