Skip to content
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.

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

HHH-17693 Cannot use 'like' operand when an AttributeConverter is used to convert the attribute in a String #7768

Merged
merged 2 commits into from Feb 8, 2024

Conversation

mbladel
Copy link
Member

@mbladel mbladel commented Feb 1, 2024

Copy link
Member

@gavinking gavinking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this looks reasonable, I guess.

Also introduce a custom `DurationJdbcType`, mainly for validation purposes.
@mbladel mbladel merged commit 50e6cb6 into hibernate:main Feb 8, 2024
24 checks passed
).getSingleResult();
assertThat( Long.parseLong( result ) ).isEqualTo( Duration.ofDays( 3 ).toMillis() );
} );
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @mbladel, these tests don't make much sense to me. How can the prefix - operator (a numeric operator) evaluate to a string? Similarly, how can the by day syntax produce a string?

I guess that the idea is that the converted field has an underlying JDBC type that's different to its Java type, and so we can do things with it that we could not do to the Java type. But that doesn't justify saying that the converter is applied recursively to all containing expressions.

So I think this undermines our type system.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that the idea is that the converted field has an underlying JDBC type that's different to its Java type, and so we can do things with it that we could not do to the Java type

Yes that was the idea, I implemented this tests specifically to ensure my changes to TypecheckUtil#assertNumeric (only used in SemanticQueryBuilder#visitUnaryExpression) and TypecheckUtil#assertDuration (only used in SemanticQueryBuilder#visitFromDurationExpression) worked as expected with converted attributes.

But that doesn't justify saying that the converter is applied recursively to all containing expressions

In both of these tests, the selected expression (SqmUnaryOperation or SqmByUnit) produces results typed using the underlying basic path, which in this case is converted thus creating a result that is also converted the same way the property itself would be. I also ran some tests and this was the case since Hibernate 6.1.

If you think this is not correct it would be definitely possible to change this behavior to account for converters and adapt these tests to new expectations, but it would be a shift in logic.

Copy link
Member

@gavinking gavinking Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so what I'm saying is I don't think it's intuitive at all that this converter acts transitively on the result of an operator expression.

Nor do I see it as something well-defined. For example, the by day operator has the effect of changing the type of the thing it's applied to (it converts an expression of type Duration to something of type Integer). So then what does it mean to apply a converter from Duration to String to the Integer result of that operation? The same objection applies to any other operator which can change the type of its operand.

So the thing here is that this was all reasonably well-defined when we were type checking comparison expressions with == or whatever, because you just checked the database-level values for equality, and the result type was always Boolean. A comparison operator never evaluates to one of its operands.

But for these operations, that's not the case, so we have to be able to answer the question of what is the result type of the operation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not at all sure what to do here.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is completely fine, by the way:

from TestEntity where convertedString like 'one%'

since it's a comparison operator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants