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.

Sign up for GitHub

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

Jump to bottom

Simplify predicates involving year function #16106

Merged

ebyhr merged 1 commit into trinodb:master from ebyhr:ebi/iceberg-year-function

Mar 17, 2023

Member

ebyhr commented Feb 14, 2023 •

edited

Loading

Description

Simplify predicates involving year function likes UnwrapDateTruncInComparison.
Continuation of #15306
Fixes #14078

Release notes

(x) Release notes are required, with the following suggested text:

# General
* Improve performance of queries that contain predicates involving `year` function. ({issue}`14078`)

cla-bot bot added the cla-signed label

ebyhr self-assigned this

ebyhr mentioned this pull request

Simplify predicates involving year function #15306

Closed

Member

martint commented Feb 14, 2023 •

edited

Loading

Rewrite year function to date_trunc for improving performance.

Why would this rewrite improve performance?

Member Author

ebyhr commented Feb 14, 2023

@martint We have UnwrapDateTruncInComparison. Updated the PR description.

ebyhr requested a review from martint

February 14, 2023 08:59

martint previously requested changes

View reviewed changes

Member

martint left a comment

That's a roundabout way of improving this specific type of expression. The optimization should go the other way -- an expression like year(date_time) = t is inherently cheaper than date_trunc('year', date_time) = ..., since the date_trunc function is more generic and expensive to compute. In general, optimizations should stand on their own (i.e., not depend on other rules to be successful). They should produce a better plan regardless of whether another optimization kicks in later.

ebyhr force-pushed the ebi/iceberg-year-function branch from 1b64348 to b9bb1ac Compare

February 15, 2023 01:00

ebyhr changed the title ~~Rewrite year function to date_trunc for improving performance~~ Simplify predicates involving year function

ebyhr force-pushed the ebi/iceberg-year-function branch from b9bb1ac to 38338cb Compare

February 15, 2023 01:09

Member Author

ebyhr commented Feb 15, 2023

@martint Changed the implementation not to depend on UnwrapDateTruncInComparison.

ebyhr requested a review from martint

February 15, 2023 08:57

findinpath self-requested a review

February 20, 2023 06:25

findinpath approved these changes

View reviewed changes

findinpath reviewed

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapYearInComparison.java Outdated

    
                          }

                          if (type instanceof TimestampType timestampType) {

                              if (timestampType.isShort()) {

                                  LocalDateTime dateTime = LocalDateTime.MAX.withYear(year).with(lastDayOfYear());

Contributor

findinpath Feb 20, 2023

Is .with(lastDayOfYear()) still necessary in the context of using LocalDateTime.MAX ?

ebyhr dismissed martint’s stale review

February 22, 2023 14:50

Addressed

findinpath requested a review from findepi

March 9, 2023 08:39

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare

March 14, 2023 11:34

findepi approved these changes

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapYearInComparison.java Outdated

    
                          }

                          if (type instanceof TimestampType timestampType) {

                              if (timestampType.isShort()) {

                                  LocalDateTime dateTime = LocalDateTime.of(year, 1, 1, 0, 0);

Member

findepi Mar 16, 2023

it's same for short and long timestamp type, so move before the if (timestampType.isShort()) check:

long yearStartEpochSecond = LocalDateTime.of(year, 1, 1, 0, 0).toEpochSecond(ZoneOffset.UTC);
long yearStartEpochMicros = multiplyExact(yearStartEpochSecond, MICROSECONDS_PER_SECOND);
if (timestampType.isShort()) {
    return yearStartEpochMicros;
}
return new LongTimestamp(yearStartEpochMicros, 0);

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapYearInComparison.java Outdated

    
                          if (type instanceof TimestampType timestampType) {

                              if (timestampType.isShort()) {

                                  LocalDateTime dateTime = LocalDateTime.of(year, 1, 1, 0, 0);

                                  return dateTime.toEpochSecond(ZoneOffset.UTC) * MICROSECONDS_PER_SECOND + LongMath.divide(dateTime.getNano(), NANOSECONDS_PER_MICROSECOND, UNNECESSARY);

Member

findepi Mar 16, 2023

dateTime.getNano() is known to be 0

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapYearInComparison.java Outdated

    
                          if (type instanceof TimestampType timestampType) {

                              if (timestampType.isShort()) {

                                  LocalDateTime dateTime = LocalDateTime.of(year, 1, 1, 0, 0);

                                  return dateTime.toEpochSecond(ZoneOffset.UTC) * MICROSECONDS_PER_SECOND + LongMath.divide(dateTime.getNano(), NANOSECONDS_PER_MICROSECOND, UNNECESSARY);

Member

findepi Mar 16, 2023

we probably should use multiplyExact

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapYearInComparison.java Outdated

    
                                  return dateTime.toEpochSecond(ZoneOffset.UTC) * MICROSECONDS_PER_SECOND + LongMath.divide(dateTime.getNano(), NANOSECONDS_PER_MICROSECOND, UNNECESSARY);

                              }

                              LocalDateTime dateTime = LocalDateTime.of(year, 1, 1, 0, 0);

                              long endInclusiveMicros = dateTime.toEpochSecond(ZoneOffset.UTC) * MICROSECONDS_PER_SECOND

Member

findepi Mar 16, 2023

endInclusiveMicros wrong var name?

core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/UnwrapYearInComparison.java Outdated

    
                          }

                          if (type instanceof TimestampType timestampType) {

                              if (timestampType.isShort()) {

                                  LocalDateTime dateTime = LocalDateTime.MAX.withYear(year).with(lastDayOfYear());

Member

findepi Mar 16, 2023

Personally i find LocalDateTime.MAX harder to reason about.
I first need to assume the max value has .999999999 second fraction (which is obvious if you know LocalDateTime internal representation) and then I need to reason how this behaves for our timestamp with precision > 9.

What about writing this more explicitly

long nextYearStartEpochSecond = LocalDateTime.of(year + 1, 1, 1, 0, 0).toEpochSecond(ZoneOffset.UTC);
long nextYearStartEpochMicros = multiplyExact(nextYearStartEpochSecond, MICROSECONDS_PER_SECOND);
if (timestampType.isShort()) {
    // TODO might be off by one
    return nextYearStartEpochMicros - scaleFactor(timestampType.getPrecision(), 6);
}
int picosOfMicro = toIntExact(PICOSECONDS_PER_MICROSECOND - scaleFactor(timestampType.getPrecision(), 12));
return new LongTimestamp(nextYearStartEpochMicros - 1, picosOfMicro);

(i hope i didn't screw the math, but not sure, hence the TODO comment; please remove the TODO)

Member

findepi Mar 16, 2023

Maybe this method should be unit-tested

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/ConstraintExtractor.java Outdated

    
                  {

                      checkArgument(constant.getValue() != null && argumentType.equals(TIMESTAMP_TZ_MICROS), "Unexpected constant: %s", constant);

                      // Normalized to UTC because for comparisons the zone is irrelevant

Member

findepi Mar 16, 2023

remove (copy&paste error)

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/ConstraintExtractor.java Outdated

    
                  private static Optional<Domain> unwrapYearInComparison(FunctionName functionName, Type argumentType, Constant constant)

                  {

                      checkArgument(constant.getValue() != null && argumentType.equals(TIMESTAMP_TZ_MICROS), "Unexpected constant: %s", constant);

Member

findepi Mar 16, 2023

we check two arguments but report only one in the message

checkArgument(constant.getValue() != null, "Unexpected constant: %s", constant);
checkArgument(type.equals(TIMESTAMP_TZ_MICROS), "Unexpected type: %s", type);

Member

findepi Mar 16, 2023 •

edited

Loading

( #16594 for where this was copied from )

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestConstraintExtractor.java Outdated

    
                  public void testExtractYearTimestampTzComparison()

                  {

                      String timestampTzColumnSymbol = "timestamp_tz_symbol";

                      FunctionCall truncateToYear = new FunctionCall(

Member

findepi Mar 16, 2023

extractYear

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestConstraintExtractor.java Outdated

    
                              List.of(new SymbolReference(timestampTzColumnSymbol)));

                      LocalDate someDate = LocalDate.of(2005, 9, 10);

                      Expression someMidnightExpression = LITERAL_ENCODER.toExpression(

Member

findepi Mar 16, 2023

yearExpression?

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestConstraintExtractor.java Outdated

Comment on lines 282 to 284

    
                      long startOfDateUtcEpochMillis = someDate.withDayOfYear(1).atStartOfDay().toEpochSecond(UTC) * MILLISECONDS_PER_SECOND;

                      LongTimestampWithTimeZone startOfDateUtc = timestampTzFromEpochMillis(startOfDateUtcEpochMillis);

                      LongTimestampWithTimeZone startOfNextDateUtc = timestampTzFromEpochMillis(someDate.plusYears(1).withDayOfYear(1).atStartOfDay().toEpochSecond(UTC) * MILLISECONDS_PER_SECOND);

Member

findepi Mar 16, 2023

startOfDate -> startOfYear


          Simplify predicates involving year function

7d5a3b5

ebyhr force-pushed the ebi/iceberg-year-function branch from 38338cb to 7d5a3b5 Compare

March 17, 2023 01:12

github-actions bot added the iceberg label

ebyhr merged commit b8967a3 into trinodb:master

ebyhr deleted the ebi/iceberg-year-function branch

March 17, 2023 05:34

github-actions bot added this to the 411 milestone

colebow mentioned this pull request

Add Trino 411 release notes #16552

Merged

hantangwangd mentioned this pull request

Unwrap functions on parameter of DataType/TimestampType used in comparison with a literal prestodb/presto#22244

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed iceberg