Remove unsound varchar->char implicit coercion

[ebyhr]

cc: @martint

[findepi]

AFAIR, the tpch/tpcds template queries need to be updated.
cc @sopel39

[sopel39]

This is NOGO as long as tpcds queries need to be changed (when comparing char table column with string literal).
We need to fix character string comparison too. One way to do it would be to have a cast dedicated for comparisons.
Some old doc about it https://docs.google.com/document/d/1UngGirjV_I7nPnIGU6G7SDS0cY8j7N_Jb3Wk3EoHChc/edit?usp=sharing

[findepi]

@sopel39 i think the conclusion was that we can fix char/varchar coercion direction only if we fix type of string literals.

@ebyhr you need to change eg 'abc' to be of type char(3) (today it's varchar(3)). cc @martint @kasiafi

[sopel39]

@sopel39 i think the conclusion was that we can fix char/varchar coercion direction only if we fix type of string literals.

wdym? We should have proper mechanism for character type comparisons in place before we flip coercion from varchar -> char

[martint]

If I recall correctly, to be able to reverse the order of char<->varchar implicit casts we first need to:

• treat string literals as char(n)
• be able to encode literals of type varchar(n) in the IR. Otherwise, PPD and other optimizations will break. For instance varchar_1_column = ‘X’ would get planned as varchar_1_column = cast(‘X’ as varchar(1)), as there's currently no way to represent a literal value of type varchar(n) in the IR.

[findepi]

• Otherwise, PPD and other optimizations will break. For instance varchar_1_column = ‘X’ would get planned as varchar_1_column = cast(‘X’ as varchar(1)), as there's currently no way to represent a literal value of type varchar(n) in the IR.

That's interesting. There isn't an assumption that LiteralEncoder produces an actual literal expression, and optimizations should not -- and they strive not -- depend on it.
Do you know which particular optimization needs a fix here?

BTW, this problem is preexisting. For instance varchar_2_column = 'X' today gets planned as varchar_2_column = cast('X' AS varchar(2)), as there's currently no direct way to represent a literal value of type varchar(n) but length shorter than n in the IR.

[kasiafi]

Do you know which particular optimization needs a fix here?

I've seen examples of code that tries to handle both "bare" literals, and literals wrapped in Cast. E.g. CanonicalizeExpressionRewriter. Also, UnwrapCasts wouldn't break, because it evaluates the expression.

However, some sites would be affected, e.g. PPD or FilterStatsCalculator.

[findepi]

However, some sites would be affected, e.g. PPD or FilterStatsCalculator.

for FilterStatsCalculator this is supposed to cover the problem

trino/core/trino-main/src/main/java/io/trino/cost/ScalarStatsCalculator.java

Lines 148 to 152 in 8e40a44

	// value is a constant
	return SymbolStatsEstimate.builder()
	.setNullsFraction(0)
	.setDistinctValuesCount(1)
	.build();

for PPD -- would be good to be more explicit about problems' addresses, as the class is too long to read at once

[kasiafi]

for PPD -- would be good to be more explicit about problems' addresses, as the class is too long to read at once

I meant the part where the predicate is pushed through a ProjectNode, and the conjuncts to push are extracted: the isInliningCandidate method. The conjunct will only be pushed if a corresponding projection assignment is based on a Literal or a SymbolReference. If we change the direction of coercions, we can get a Cast where we used to have a Symbol. However, it's a symmetric situation. Sometimes we'll get a Symbol where we used to have a Cast. I think we should consider which direction of the char <-> varchar coercion is going to produce less casts on non-constants.

[kasiafi]

for FilterStatsCalculator this is supposed to cover the problem

I meant FilterStatsCalculator, not ScalarStatsCalculator:

trino/core/trino-main/src/main/java/io/trino/cost/FilterStatsCalculator.java

Line 388 in 8e40a44

if (right instanceof Literal) {

[findepi]

I meant FilterStatsCalculator, not ScalarStatsCalculator:

FilterStatsCalculator relies on ScalarStatsCalculator, that's why l linked a hopefully-relevant piece of ScalarStatsCalculator.

I meant the part where the predicate is pushed through a ProjectNode, and the conjuncts to push are extracted: the isInliningCandidate method. The conjunct will only be pushed if a corresponding projection assignment is based on a Literal or a SymbolReference.

Thanks!
Sounds like pre-existing problem though, i.e. candidate for improvement that we can make today, and we may need to do prior to landing the coercion direction swap.

[findepi]

Sounds like pre-existing problem though, i.e. candidate for improvement that we can make today, and we may need to do prior to landing the coercion direction swap.

#10663

[ebyhr]

Let me close because there are blockers (e.g. "treat string literals as char(n)"). Thanks for reviewing the PR.