Skip literal vs round tripped comparison for non-orderable types in UnwrapCastInComparison

[JunhyungSong]

Skip literal vs round tripped comparison for non-orderable types in UnwrapCastInComparison

• Non-orderable type allows only equality checks, so round tripped comparison cannot be applicable

Description

Additional context and related issues

Release notes

(v ) 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}`issuenumber`)

[martint]

The optimization works for non-orderable types, too, when they are “equatable” (they support equals/not-equals).

What problem are you trying to solve?

BTW, you can read more about the optimization here: https://trino.io/blog/2019/05/21/optimizing-the-casts-away.html

[JunhyungSong]

The optimization works for non-orderable types, too, when they are “equatable” (they support equals/not-equals).

What problem are you trying to solve?

BTW, you can read more about the optimization here: https://trino.io/blog/2019/05/21/optimizing-the-casts-away.html

The simple query below,

select * from (select cast(row(
    MAP(ARRAY['foo', 'bar'], ARRAY[1, 2])
  ) as row(a MAP(VARCHAR(3), BIGINT))) as row_column)
where row_column = row(
    MAP(ARRAY['foo', 'bar'], ARRAY[1, 2])
  )

fails because map type is not orderable. Because UnwrapCastInComparison calls getComparisonUnorderedLastOperator in the compare function.

[martint]

The proper fix would be to check if the type is not orderable but equatable, skip the literalVsRoundtripped > 0 and literalVsRoundtripped < 0 branches and just perform the corresponding logic depending on whether the value is equal or not.

[JunhyungSong]

The proper fix would be to check if the type is not orderable but equatable, skip the literalVsRoundtripped > 0 and literalVsRoundtripped < 0 branches and just perform the corresponding logic depending on whether the value is equal or not.

Will try.

[JunhyungSong]

@martint can you review, please? Thanks.

[martint]

This is unnecessary. It can be made to support row types by changing the query to do this, instead:

FROM (VALUES CAST(ROW(%s) AS row(%s))) t(v)

[martint]

This should never happen. If the type of the right term of the comparison expression were not orderable or comparable, the ComparisonExpression would be invalid.

[JunhyungSong]

Right. This is an additional check arguments like other check arguments as safeguards since this should not add any overheads. Also, I think this is helpful to understand the codes. I can change it to throw instead of silently return. Or do you think it's totally superfluous?

[martint]

Yes, it's superfluous. There's an assumption everywhere in the optimizer that the plan that is provided to each rule is well-formed. It's unreasonable to expect that every rule has to validate that structure. Otherwise, we'll end up with these types of checks sprinkled all over the code and we'll end up with code that's much harder to read and follow.

[martint]

This is unnecessary, too. It's effectively validating that the ComparisonExpression that was passed to this method is valid, but that's an assumption we don't need to check.

[JunhyungSong]

This is also a safeguard. In case there is a bug in parser/checker, it would be easier to understand what the cause is. And this also improves the code readability. But, if you strongly believe that this is not beneficial at all, I'm fine with deleting it.

[JunhyungSong]

The test failures seem irrelevant. Can you merge this if you don't have more comments, please?